Closed Bug 927397 Opened 11 years ago Closed 11 years ago

Refactor repository structure for different application types

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(firefox25 fixed, firefox26 fixed, firefox27 fixed, firefox28 fixed, firefox-esr17 wontfix, firefox-esr24 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox25 --- fixed
firefox26 --- fixed
firefox27 --- fixed
firefox28 --- fixed
firefox-esr17 --- wontfix
firefox-esr24 --- fixed

People

(Reporter: AndreeaMatei, Assigned: AndreeaMatei)

References

(
URL
)

Details

(Whiteboard: [metro])

Attachments

(5 files, 8 obsolete files)

patch v4.3
310.09 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
[aurora] patch v1
310.42 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
[beta, release] patch v2
304.98 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
[ESR24] patch v1
304.85 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
followup Release v2
1.67 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review 
We want to change structure of the repository in order to add the metro side into it. So we've decided to have it like this:

|-data
|-firefox
  |-lib
  |-tests
|-lib
|-metrofirefox
  |-lib
  |-tests

In lib we'll have all the common libraries (like assertions, dom-utils), data will remain the same.
The tests in the current repository will just be moved inside firefox directory.

This will be blocked until we can update the automation scripts as well, so we can still do testruns each day.
This bug is one of the blockers for us to get Metro tests running. So it has to become P1.
Priority: -- → P1
Attached patch patch v1 (obsolete) — — Splinter Review 
Giving that it's such a huge change, I added you all to review.

I have created some readme.txt files in the metrofirefox directories in order to be able to create them inside the patch. We can remove those later if needed.
Also updated the readme files to point to the MDN page and the #automation channel.

The current UI libraries (private browsing and blocklist) are not applying in the general lib. There I have left assertions, dom-utils and places as it contains some general APIs.

Reports under OS X (I tested all):
Remote testrun:
http://mozmill-crowd.blargon7.com/#/remote/report/f54ca65f73fd56845c30b7ab7dd0ea16
http://mozmill-crowd.blargon7.com/#/remote/report/f54ca65f73fd56845c30b7ab7dd0e5fa

Functional testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7dd4326e
http://mozmill-crowd.blargon7.com/#/functional/report/f54ca65f73fd56845c30b7ab7dd3af64

Addons testrun:
http://mozmill-crowd.blargon7.com/#/addons/report/f54ca65f73fd56845c30b7ab7dd0dc62

Update testrun:
http://mozmill-crowd.blargon7.com/#/update/report/f54ca65f73fd56845c30b7ab7dd0fb27
http://mozmill-crowd.blargon7.com/#/update/report/f54ca65f73fd56845c30b7ab7dd0f671

L10n testrun:
http://mozmill-crowd.blargon7.com/#/l10n/report/f54ca65f73fd56845c30b7ab7dd8a46d

Endurance testrun:
http://mozmill-crowd.blargon7.com/#/endurance/report/f54ca65f73fd56845c30b7ab7ddd85ac
Attachment #819766 - Flags: review?(hskupin)
Attachment #819766 - Flags: review?(dave.hunt)
Attachment #819766 - Flags: review?(andrei.eftimie)
Comment on attachment 819766 [details] [diff] [review]
patch v1

Review of attachment 819766 [details] [diff] [review]:
-----------------------------------------------------------------

I think that looks fantastic and I can feel how much work this was. So lets make sure we also focus on the automation scripts and their support now, and get this patch in once support for the new locations has been landed for both Mozmill 1.5 and 2.0.
Attachment #819766 - Flags: review?(hskupin) → review+
Comment on attachment 819766 [details] [diff] [review]
patch v1

Review of attachment 819766 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but we are missing one important thing:

We're losing all history this way, we should use hg's addremove option to tell mercurial that the files have been moved.
Right now we're seeing them as deleted and new file added.

I've just run a test:
> patch -p1 < ../patches/restructure.patch
> hg addremove --similarity 85 // 100 will only get identical files, we change the path so some libs, so some fuzzyness is needed for that
> hg status -C // this shows you which files were identified as renamed / moved
> hg commit // or qnew should work

This yields a patch that correctly references the "new" files to their previous location, and we preserve all history.
Attachment #819766 - Flags: review?(andrei.eftimie) → review-
Attachment #819766 - Flags: review?(dave.hunt)
Summary: Update repository structure by dividing it in application types → Refactor repository structure for different application types
The patch doesn't apply anymore for a lot of tests, and would need an overhaul. At some point we might want to close the default branch for this landing. But lets get in support for the scripts first and a new env shipped.
I'll have the patch updated by tomorrow at noon, I'm currently testing it.
Attached patch patch v2 (obsolete) — — Splinter Review 
Unfortunately, this addremove --similarity will check the contents but the path it's not persisted:
recording removal of tests\endurance\testFlash_SWFVideoEmbed\manifest.ini as rename to firefox\tests\l10n\testCropped\manifest.ini (100% similar)

I think somehow this is messing up the structure, my patch was ready as suggested by Andrei but when I applied it on a clean repo I got a tests directory left with different empty testname directory inside. And I've tried this several times, also tried to remake the patch with addremove. 

I'm adding the patch in the same format as the previous.

Tested it on window. If it's ok, Andrei, please make sure this gets tested on os x and linux as well, just for sanity.

Reports:
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/d17690a112360a2b3155acaa2730154c

Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/d17690a112360a2b3155acaa27300791
http://mozmill-crowd.blargon7.com/#/functional/report/d17690a112360a2b3155acaa273001f8

Remote:
http://mozmill-crowd.blargon7.com/#/remote/report/d17690a112360a2b3155acaa272f84a9
http://mozmill-crowd.blargon7.com/#/remote/report/d17690a112360a2b3155acaa272e9c09

Addons:
http://mozmill-crowd.blargon7.com/#/addons/report/d17690a112360a2b3155acaa27307e9d

Update:
http://mozmill-crowd.blargon7.com/#/update/report/d17690a112360a2b3155acaa27306e70
http://mozmill-crowd.blargon7.com/#/update/report/d17690a112360a2b3155acaa273067e2
Attachment #827424 - Flags: review?(hskupin)
Attachment #827424 - Flags: review?(dave.hunt)
Attachment #827424 - Flags: review?(andrei.eftimie)
Attachment #819766 - Attachment is obsolete: true
(In reply to Andreea Matei [:AndreeaMatei] from comment #7)
> Unfortunately, this addremove --similarity will check the contents but the
> path it's not persisted:
> recording removal of tests\endurance\testFlash_SWFVideoEmbed\manifest.ini as
> rename to firefox\tests\l10n\testCropped\manifest.ini (100% similar)

Those files are identical, that's why it is seeing them as identical.
We can manually tweak the final outcome.

> I think somehow this is messing up the structure, my patch was ready as
> suggested by Andrei but when I applied it on a clean repo I got a tests
> directory left with different empty testname directory inside. And I've
> tried this several times, also tried to remake the patch with addremove. 

I will recheck and try changing this latest patch with addremove.
Or maybe remake it directly with hg mv

> I'm adding the patch in the same format as the previous.

We really can't land it in this format. We need to preserve history.
(In reply to Andrei Eftimie from comment #8)
> (In reply to Andreea Matei [:AndreeaMatei] from comment #7)
> > Unfortunately, this addremove --similarity will check the contents but the
> > path it's not persisted:
> > recording removal of tests\endurance\testFlash_SWFVideoEmbed\manifest.ini as
> > rename to firefox\tests\l10n\testCropped\manifest.ini (100% similar)
> 
> Those files are identical, that's why it is seeing them as identical.
> We can manually tweak the final outcome.
If it's coming to that, might as well use hg mv, seems less trouble than searching through a huge patch and correct all the mistakes addremove has done. 
> 
> > I'm adding the patch in the same format as the previous.
> 
> We really can't land it in this format. We need to preserve history.
I understand, but addremove is not preserving any history if it's twisting all the right path files.
Attached patch patch v3 (obsolete) — — Splinter Review 
Updated using hg mv. Andrei, please ask the team to test this, thanks!
Attachment #827424 - Attachment is obsolete: true
Attachment #827424 - Flags: review?(hskupin)
Attachment #827424 - Flags: review?(dave.hunt)
Attachment #827424 - Flags: review?(andrei.eftimie)
Attachment #827896 - Flags: review?(hskupin)
Attachment #827896 - Flags: review?(dave.hunt)
Attachment #827896 - Flags: review?(andrei.eftimie)
Looks much better with renames.

I have 1 conflict when applying the patch in 
> firefox/tests/functional/testGeolocation/testShareLocation.js

Please check this
Comment on attachment 827896 [details] [diff] [review]
patch v3

Review of attachment 827896 [details] [diff] [review]:
-----------------------------------------------------------------

The l10n tests have not been run and those seem to be broken for me.
Attachment #827896 - Flags: review?(hskupin)
Attachment #827896 - Flags: review?(dave.hunt)
Attachment #827896 - Flags: review?(andrei.eftimie)
Attachment #827896 - Flags: review-
Attached patch patch v4 (obsolete) — — Splinter Review 
Updated, everything works well now.

Partial reports, still running windows.
Linux:
  l10n: 
    http://mozmill-crowd.blargon7.com/#/l10n/report/d7f12eb72275b30155ba68b8bc564da9
  addons:
    http://mozmill-crowd.blargon7.com/#/addons/report/d7f12eb72275b30155ba68b8bc56692b
  functional:
    http://mozmill-crowd.blargon7.com/#/functional/report/d7f12eb72275b30155ba68b8bc567725
    http://mozmill-crowd.blargon7.com/#/functional/report/d7f12eb72275b30155ba68b8bc567257
  remote: 
    http://mozmill-crowd.blargon7.com/#/remote/report/d7f12eb72275b30155ba68b8bc56601d
    http://mozmill-crowd.blargon7.com/#/remote/report/d7f12eb72275b30155ba68b8bc565729
  update:
    http://mozmill-crowd.blargon7.com/#/update/report/d7f12eb72275b30155ba68b8bc56f22d
    http://mozmill-crowd.blargon7.com/#/update/report/d7f12eb72275b30155ba68b8bc56d7f9
  endurance:
    http://mozmill-crowd.blargon7.com/#/endurance/report/d7f12eb72275b30155ba68b8bc56ca8e
OS X:
  l10n:
    http://mozmill-crowd.blargon7.com/#/l10n/report/d7f12eb72275b30155ba68b8bc568542
  addons:
    http://mozmill-crowd.blargon7.com/#/addons/report/d7f12eb72275b30155ba68b8bc5692b3
  functional:
    http://mozmill-crowd.blargon7.com/#/functional/report/d7f12eb72275b30155ba68b8bc56e581
    http://mozmill-crowd.blargon7.com/#/functional/report/d7f12eb72275b30155ba68b8bc57eb2f
  remote:
    http://mozmill-crowd.blargon7.com/#/remote/report/d7f12eb72275b30155ba68b8bc56c646
    http://mozmill-crowd.blargon7.com/#/remote/report/d7f12eb72275b30155ba68b8bc56ac25
  update:
    http://mozmill-crowd.blargon7.com/#/update/report/d7f12eb72275b30155ba68b8bc5a56ad
    http://mozmill-crowd.blargon7.com/#/update/report/d7f12eb72275b30155ba68b8bc59cbfb
Attachment #827896 - Attachment is obsolete: true
Attachment #832238 - Flags: review?(hskupin)
Attachment #832238 - Flags: review?(dave.hunt)
Attachment #832238 - Flags: review?(andrei.eftimie)
This looks really good at a first glance.

I'll be running all testruns onder OSX this evening so we'll have full coverage across OS'es
Comment on attachment 832238 [details] [diff] [review]
patch v4

Review of attachment 832238 [details] [diff] [review]:
-----------------------------------------------------------------

I think that looks fine. Everything passes for me. r=me in case we solved the 'keeping history' issue Andrei mentioned.
Attachment #832238 - Flags: review?(hskupin) → review+
Thanks. Yes, the history part was solved, not with --similarity but with hg move/rename.
Moved tests and lib folder and then just the 3 libs needed in the general lib folder got copied back.

It'll need a small update due to some landins from today, but nothing major. I was planning to do that after we get all reviews, to block the branch afterwards.
Attached patch patch v4.1 (obsolete) — — Splinter Review 
Updated due to a small change to the branch (lib/tests/testToolbar from Mihaela's work on bug 939107). I believe there's nothing more to be landed until tomorrow.
Attachment #832238 - Attachment is obsolete: true
Attachment #832238 - Flags: review?(dave.hunt)
Attachment #832238 - Flags: review?(andrei.eftimie)
Attachment #8334477 - Flags: review?(andrei.eftimie)
Checked backporting, the change mentioned above landed only on default, so the other branches will need to have it removed. That's the only change for aurora/beta/release. Esr24 has a few tests with conflicts, but should be quick to fix.
Not sure if we want to change esr17 as well, giving that we will soon remove it completely and our automation script supports both paths at the moment.
Comment on attachment 8334477 [details] [diff] [review]
patch v4.1

Review of attachment 8334477 [details] [diff] [review]:
-----------------------------------------------------------------

I think we can move over more libraries to the general lib folder.

1) utils.js
2) screenshots.js
3) graphics.js
4) endurance.js
5) stack.js
6) prefs.js (we'll have to split it up into API vs UI, I'm assuming we'll want/need to set prefs in metro as well)

I don't see any of these as blocking us right now, most might need a bit of rework (or split functionality up in case of utils.js)
Some will need work (utils.js, prefs.js), while some might need almost none (stack.js?, graphics.js?)

So lets add followup bugs to investigate and do these changes where they are needed.

I've ran all testruns on OSX without any problems with this patch applied.
Attachment #8334477 - Flags: review?(andrei.eftimie) → review+
I can move stack.js and graphics.js now if you want me to, regarding the others modules, another bug would be more appropriate as it's a lot of work in splitting them. 
Not sure at this time about endurance and prefs, but will be up for discussion, I'll file the bug.
Blocks: 940917
Attached patch patch v4.2 (obsolete) — — Splinter Review 
Moved stack.js and graphics.js back in the general lib.
Attachment #8336043 - Flags: review?(andrei.eftimie)
Comment on attachment 8336622 [details] [diff] [review]
patch v4.3

Review of attachment 8336622 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

We've run this against mozmill 2.0.1 as well, and it works fine.
Attachment #8336622 - Flags: review?(andrei.eftimie) → review+
We re-triggered all tests for a mozilla-central build from yesterday and all is passing. That means we could backport this patch to Aurora today. For beta we might wait once we get green light from Juan.

Juan, what is the schedule today regarding beta testing? Which type of tests have to be run?
Flags: needinfo?(jbecerra)
Attachment #8336622 - Flags: checkin+
Comment on attachment 8336771 [details] [diff] [review]
[aurora] patch v1

Review of attachment 8336771 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/f110d3266ca7 (mozilla-aurora)
Attachment #8336771 - Flags: review?(andrei.eftimie)
Attachment #8336771 - Flags: review+
Attachment #8336771 - Flags: checkin+
As I have read we will most likely release 26.0b7 on Monday. So update tests will have to be performed later on that day. That should give us enough room to backport the patch to beta and release.
Flags: needinfo?(jbecerra)
Attached patch [beta, release] patch v1 (obsolete) — — Splinter Review 
Applies to beta and release, tested.
Attachment #8337724 - Flags: review?(andrei.eftimie)
Comment on attachment 8337724 [details] [diff] [review]
[beta, release] patch v1

Review of attachment 8337724 [details] [diff] [review]:
-----------------------------------------------------------------

You missed some files that need an update to the assertion path:
> firefox/tests/functional/testPreferences/testPaneRetention.js
> firefox/tests/functional/testTabbedBrowsing/testNewTab.js

They'll both fail with:
> "Module \"../../../lib/assertions\" not found"
Attachment #8337724 - Flags: review?(andrei.eftimie) → review-
Comment on attachment 8338355 [details] [diff] [review]
[beta, release] patch v2

Review of attachment 8338355 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
r+ from me

We have a big queue in Jenkins ATM on the beta branch.
We should land this only after we finish today's Beta runs.
Attachment #8338355 - Flags: review?(andrei.eftimie) → review+
Landed backports:
http://hg.mozilla.org/qa/mozmill-tests/rev/b5ff39d597d2 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/553e8cb3060e (mozilla-release)

We need to land this on ES24 as well, then we're done.
status-firefox25: affected → fixed
status-firefox26: affected → fixed
Attachment #8338355 - Flags: checkin+
If esr17 wouldn't need major changes I would even prefer to have it there. But only then.
Attached patch followup Release (obsolete) — — Splinter Review 
Missed to move testAboutCache as well, since we removed it from beta up in bug 888924.

Report:
http://mozmill-crowd.blargon7.com/#/remote/report/b99421c0f132c68dec1548288a03d773
Attachment #8339143 - Flags: review?(andrei.eftimie)
Comment on attachment 8339143 [details] [diff] [review]
followup Release

Review of attachment 8339143 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine. Please get it landed.
Attachment #8339143 - Flags: review?(andrei.eftimie) → review+
Flipped the lines in the manifest to be alphabetically sorted.
Attachment #8339143 - Attachment is obsolete: true
Attachment #8339213 - Flags: review?(andrei.eftimie)
Comment on attachment 8339213 [details] [diff] [review]
followup Release v2

Review of attachment 8339213 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/5c9e62a30e00 (mozilla-release)
Attachment #8339213 - Flags: review?(andrei.eftimie)
Attachment #8339213 - Flags: review+
Attachment #8339213 - Flags: checkin+
Comment on attachment 8339212 [details] [diff] [review]
[ESR24]  patch v1

Review of attachment 8339212 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/a9d5e65e0766 (mozilla-esr24)
Attachment #8339212 - Flags: review?(andrei.eftimie)
Attachment #8339212 - Flags: review+
Attachment #8339212 - Flags: checkin+
I think we're done here.

Great work Andreea!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox-esr24: affected → fixed
Resolution: --- → FIXED
Agreed. Thanks for all that work involved here. Please send an email to the mailing list, so everyone is aware of it.
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: