Closed Bug 632893 Opened 9 years ago Closed 9 years ago

Remove useless Cc/Ci/Cr/Cu/Components.*/Services.jsm from Firefox tests

Categories

(Firefox :: General, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

Attachments

(5 files)

I started to work on SeaMonkey bug 632860,
then I noticed (for example)
http://hg.mozilla.org/mozilla-central/rev/17a573a66469
which makes definitions local to a function while yet using them in a newly-added one.
This (still) works: obviously the definitions are useless (as Firefox already provide them globally, iiuc) :-/

"Found 25 matching lines in 16 files"
Let's start with Places Chrome...
Attachment #511100 - Flags: review?(mak77)
Summary: Remove useless Cc/Ci/Cr/Services.jsm from /browser tests → Remove useless Cc/Ci/Cr/Cu/Services.jsm from /browser tests
Attachment #511100 - Flags: review?(mak77) → review+
Comment on attachment 511100 [details] [diff] [review]
(Av1) /components/places/tests/chrome/
[Checked in: Comment 2]

http://hg.mozilla.org/mozilla-central/rev/4599855d0cbd
Attachment #511100 - Attachment description: (Av1) /components/places/tests/chrome/ → (Av1) /components/places/tests/chrome/ [Checked in: Comment 2]
Comment on attachment 512986 [details] [diff] [review]
(Bv1) /components/sessionstore/test/browser/
[Checked in: Comment 5]

I guess Services.jsm has already been imported. Make sure tests pass before pushing (and check with the sheriff before pushing - this can wait, blockers can't).
Attachment #512986 - Flags: review?(paul) → review+
Comment on attachment 512986 [details] [diff] [review]
(Bv1) /components/sessionstore/test/browser/
[Checked in: Comment 5]

http://hg.mozilla.org/mozilla-central/rev/ebc51d671b39
Attachment #512986 - Attachment description: (Bv1) /components/sessionstore/test/browser/ → (Bv1) /components/sessionstore/test/browser/ [Checked in: Comment 5]
Serge, when you're doing this stuff, you should put DONTBUILD in the commit message so you don't waste build time.
(In reply to comment #6)
> Serge, when you're doing this stuff, you should put DONTBUILD in the commit
> message so you don't waste build time.

(though to be fair, any patch for whom it's worth suggesting "Make sure tests pass before pushing" (comment 4) is probably worth giving some test runs on tinderbox)
(In reply to comment #6)
> Serge, when you're doing this stuff, you should put DONTBUILD in the commit
> message so you don't waste build time.

I agree, but I got complains before (as I'm used to), so I'm not doing it anymore :-|
You won't get complaints if you don't break anything. Always push to try first.
(In reply to comment #9)
> You won't get complaints if you don't break anything.

That proved to be false multiple times... :-<
Attachment #514094 - Flags: review?(mak77) → review+
Comment on attachment 514094 [details] [diff] [review]
(Cv1) /components/places/tests/browser/
[Checked in: Comment 12]

http://hg.mozilla.org/mozilla-central/rev/b6c2dbc631f7
Attachment #514094 - Attachment description: (Cv1) /components/places/tests/browser/ → (Cv1) /components/places/tests/browser/ [Checked in: Comment 12]
Summary: Remove useless Cc/Ci/Cr/Cu/Services.jsm from /browser tests → Remove useless Cc/Ci/Cr/Cu/Services.jsm from Firefox tests
Comment on attachment 514381 [details] [diff] [review]
(Dv1) /components/privatebrowsing/test/*
[Checked in: Comment 17]

while the changes are trivial, you probably want ehsan to review stuff in PB component.
Attachment #514381 - Flags: review?(mak77) → review?(ehsan)
Comment on attachment 514381 [details] [diff] [review]
(Dv1) /components/privatebrowsing/test/*
[Checked in: Comment 17]

r=me given the assumption that these pass on the try server.
Attachment #514381 - Flags: review?(ehsan) → review+
(In reply to comment #15)
> r=me given the assumption that these pass on the try server.

Succeeded:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=caad6d294299
Comment on attachment 514381 [details] [diff] [review]
(Dv1) /components/privatebrowsing/test/*
[Checked in: Comment 17]

http://hg.mozilla.org/mozilla-central/rev/881f4ab63c31
Attachment #514381 - Attachment description: (Dv1) /components/privatebrowsing/test/* → (Dv1) /components/privatebrowsing/test/* [Checked in: Comment 17]
Summary: Remove useless Cc/Ci/Cr/Cu/Services.jsm from Firefox tests → Remove useless Cc/Ci/Cr/Cu/Components.*/Services.jsm from Firefox tests
Blocks: 636551
Shawn, could you check
http://hg.mozilla.org/try/diff/fb183726accd/browser/components/places/tests/perf/test_perf_large_delete.xul

Ftr, your bug 432706 comment 24:
{
Shawn Wilsher :sdwilsh      2008-07-18 12:15:53 PDT

What an improvement.  Now running that perf test it takes less than one second
to complete with this patch.
}

Then I wonder what should be done with this test:
*remove the whole test as obsolete?
*remove startProfiling()/stopProfiling() calls only?
*keep |document.getElementById("test-result").value = ...|?
*not use any SimpleTest.ok() or the like?
*...

Thanks.
(In reply to comment #18)
> Then I wonder what should be done with this test:
Why was it renamed in the first place?  When it didn't have the test_ prefix, it wasn't ran on tinderbox but was still runnable as a standalone test.
(In reply to comment #19)
> (In reply to comment #18)
> > Then I wonder what should be done with this test:
> Why was it renamed in the first place?  When it didn't have the test_ prefix,
> it wasn't ran on tinderbox but was still runnable as a standalone test.

It is renamed in this WIP-patch because
1) 'python -u mochitest/runtests.py [...] --chrome --test
-path=browser/components/places/tests/perf/perf_large_delete.xul' didn't seem to work for me.
2) that was required to get it run on Try!
3) I wanted to show (you) what the change could be/mean: hence my question(s).
4) even if its body should still not be run automatically, I think it could be easier to use a todo() rather than excluding the test...
(In reply to comment #20)
> 1) 'python -u mochitest/runtests.py [...] --chrome --test
> -path=browser/components/places/tests/perf/perf_large_delete.xul' didn't seem
> to work for me.
make mochitest-browser-chrome TEST_PATH=browser/components/places/
tests/perf/perf_large_delete.xul *used* to work.  It's clearly broken now ):
I'm starting to like this bug less and less as it goes on.  This has landed multiple patches in a single bug, it's doing way more than it's advertizing (it's re-enabling tests in attachment 514982 [details] [diff] [review]), and it's getting more and more confusing by the minute.  :(
(In reply to comment #23)

> This has landed multiple patches in a single bug,

Sure! And there are more to come...

> it's doing way more than it's advertizing

Am I? It's all about removing redundant code wrt to "Cx" variables and Services.jsm (full) use.

> (it's re-enabling tests in attachment 514982 [details] [diff] [review]),
> and it's getting more and more confusing by the minute.  :(

On comment 15, you asked for a Try run.
Now, you complain about making that easier to do (and fixing result logs).
Talk about confusion :-/
why do you think that an early return in a test is more visible than an explicit comment in a Makefile? I think the opposite, I often found disabled tests thanks to the Makefile, since I hardly open all test files to check if they are enabled. And due to the number of tests noticing a TODO is not that easy (but maybe I'm missing something that would allow me to notice it easily?)
(In reply to comment #25)

> why do you think that an early return in a test is more visible than an
> explicit comment in a Makefile?

It's the early return, and even more the todo().
Because Makefile are build-time, whereas todo() (+ return) is run-time.

Also, it's more obvious when looking at test sources through MXR or Hg: no need to check Makefile too.

> I think the opposite, I often found disabled tests thanks to the Makefile,

Makefile might be good for developers, but not for (automatic) testers.
I think a test should only be excluded from the build if it brakes the latter.

> since I hardly open all test files to check if they are enabled.

Sure: that's why adding a todo() when early returning helps to know what is going on. (and this implies tests are built.)

> And due to the number of tests noticing a TODO is not that
> easy (but maybe I'm missing something that would allow me to notice it easily?)

Well, manually searching all makefiles for disabled tests is certainly harder than an automatic search for "KNOWN-FAIL" on result logs.
Also, bugs should be filed (so they can be searched for), just as I filed bug 636551.
For example, look at how reftests/crashtests are documented (now), in their .list files.

To summarize, my rule would be "BugZilla should be preferred to build results, and the latter preferred to Makefiles" when possible.
(In reply to comment #26)
> Because Makefile are build-time, whereas todo() (+ return) is run-time.

Note that it's even worth when tests are commented out rather than through a |($warning ...)| or the like!
We're done here.

Long, multipatch fixes for trivial (and, as far as I can tell, harmless) issues are a waste of reviewer time, and just add risk to the tree. I'll note that you've repeatedly caused test failures by pushing untested changes -- which wastes even more of people's time.

Consider further changes to this bug and similar issues WONTFIX. Closing this as FIXED because it's already landed stuff.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 514982 [details] [diff] [review]
(Ev1) /components/places/tests/perf/, Be more explicit about skipping browser_ui_history_menu.js and browser_ui_locationbar.js

Clearing this review request. If there's a problem with the state of these tests from previous patches in this bug, please back them out instead.
Attachment #514982 - Flags: review?(mak77)
You need to log in before you can comment on or make changes to this bug.