896029 - We should warn people against running tests on their main profile, or kill that feature

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect, P3)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

iirc it is possible to run tests on your main profile which I have done in the past, but with tests like we have for places which reset history and bookmarks we should really either warn people against using the -p flag or disable it.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

I think there are advantages to `cfx test -p` tho, like if you want to see if our modules work with the XYZ add-on.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Agreed -- this is dangerous, as I wouldn't want my profile to be nuked, which is the only way to get a fresh state.

Perhaps instead of warning, we can copy the profile first, and perform the tests on the copy?

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Agreed -- this is dangerous, as I wouldn't want my profile to be nuked,
> which is the only way to get a fresh state.
> 
> Perhaps instead of warning, we can copy the profile first, and perform the
> tests on the copy?

It's not really a true test, you'd want to:

1. copy profile
2. keep running on the current profile
3. ???
4. if you haven't segfaulted yet and reach an exit condition, copy back the copied profile to the original spot.

The user experience *I'd* want is that we know somehow that the current profile has been created for testing only, and skip these destructive tests if this is not true, with red warnings.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

> 4. if you haven't segfaulted yet and reach an exit condition, copy back the
> copied profile to the original spot.

I wouldn't want the profile copied back -- that'd just be the original modified by the tests, which wouldn't seem too useful. The benefit I see for copying a profile is there may be issues that only arise with some combination of preferences and previously installed add-ons.

Skipping destructive tests doesn't seem reliable (seems to defeat the purpose) -- and we could also add global configurations (like the devtools debugging error messages recently added).

What's the drawback from performing tests on a cloned profile?

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)

> What's the drawback from performing tests on a cloned profile?

Just that the developer's profile might be weirdly broken and create extra work.

I suppose if we marked the cloned profile as well and would only run on profiles that are either cloned or newly created, that works. Importantly, we should never run these destructive tests on a profile unless we're sure it has either been cloned or newly created.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

We can just run cfx test on temporary copy as Jordan suggested. Not sure how important this is though, kind of expect that people won't run sdk tests on their actual profile.

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #6)
> We can just run cfx test on temporary copy as Jordan suggested. Not sure how
> important this is though, kind of expect that people won't run sdk tests on
> their actual profile.

Ya I'm leaning towards using a warning for now at least, I doubt we'll hear any issues from anyone, and people that really want to test against their special profile can make their own copy.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #6)
> > We can just run cfx test on temporary copy as Jordan suggested. Not sure how
> > important this is though, kind of expect that people won't run sdk tests on
> > their actual profile.
> 
> Ya I'm leaning towards using a warning for now at least, I doubt we'll hear
> any issues from anyone, and people that really want to test against their
> special profile can make their own copy.

It's not cheap to copy an entire profile folder, can take minutes on real world users. We discussed this previously in bug 730369 in the context of "cfx test" but the issue raised here is more about cfx testpkgs, right? I don't see a reason why those should use the user's profile, in fact I see lots of reasons why it shouldn't so I'm totally for blocking that.

We've not heard many complaints and this is a largely hidden option though so I'm not inclined to give this a high priority.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend (:Mossop) from comment #8)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> > comment #6)
> > > We can just run cfx test on temporary copy as Jordan suggested. Not sure how
> > > important this is though, kind of expect that people won't run sdk tests on
> > > their actual profile.
> > 
> > Ya I'm leaning towards using a warning for now at least, I doubt we'll hear
> > any issues from anyone, and people that really want to test against their
> > special profile can make their own copy.
> 
> It's not cheap to copy an entire profile folder, can take minutes on real
> world users. We discussed this previously in bug 730369 in the context of
> "cfx test" but the issue raised here is more about cfx testpkgs, right?

Yes

> I don't see a reason why those should use the user's profile, in fact I see
> lots of reasons why it shouldn't so I'm totally for blocking that.
> 
> We've not heard many complaints and this is a largely hidden option though
> so I'm not inclined to give this a high priority.

I'd rather not remove the feature, if that's what you mean by "blocking that" it helps while developing modules to be able to examine a profile after running tests on it, and it helps to have the profile in a place that is easy to get to instead of wherever the auto generated one is stored.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

This problem is nullified by bug 915376, wontfix?

Comment 11

[Dave Townsend [:mossop]]

Are we really no longer going to have a way of running tests using a specific profile in the future?

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend (:Mossop) from comment #11)
> Are we really no longer going to have a way of running tests using a
> specific profile in the future?

If we do then it would be in the jpm tool, I don't think updating the cfx tool with this warning is worth while though.

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

Sorry we won't be releasing any new versions of cfx, jpm is the replacement https://www.npmjs.com/package/jpm