Shorten file path length of mochitest

NEW
Unassigned

Status

()

Core
Build Config
7 years ago
4 years ago

People

(Reporter: rail, Unassigned)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

7 years ago
Releng hit a problem while building mozilla-central on windows. The error message is:

Could not create the directory:
..\..\..\..\..\..\_tests\testing\mochitest\tests\parser\htmlparser\tests\mochitest\html5lib_tree_construction\scripted:
No such file or directory

It would be great to shorten the path of mochitest.
(Reporter)

Comment 1

7 years ago
Created attachment 495060 [details]
shorteh.sh

A simple bash script replacing hardcoded paths.

It scans Makefile.in files only. Other 2 files will be patched in the next patch.
Attachment #495060 - Flags: review?(ted.mielczarek)
(Reporter)

Comment 2

7 years ago
Created attachment 495061 [details] [diff] [review]
Manual fixes
Attachment #495061 - Flags: review?(ted.mielczarek)
Summary: Shorten file patch length of mochitest → Shorten file pach length of mochitest
Summary: Shorten file pach length of mochitest → Shorten file path length of mochitest
(Reporter)

Comment 3

7 years ago
Created attachment 495425 [details] [diff] [review]
Manual fixes

Files under testing/mochitest require additional intervention, I realized after a try build.

* js/src/config/config.mk synced with config/config.mk
* Adjusted relativesrcdir for files in testing/mochitest

This patch should be applied after running shorten.sh

There are 6 oranges, hopefully unrelated to the changes made by the patch. It would be great to recheck this.

Here is a summary of the oranges: http://pastebin.mozilla.org/877061
Attachment #495061 - Attachment is obsolete: true
Attachment #495425 - Flags: review?(ted.mielczarek)
Attachment #495061 - Flags: review?(ted.mielczarek)
rail, I think your two Manual Fixes patches are disjoint and we would need them both? am I wrong?
(Reporter)

Comment 5

7 years ago
Created attachment 495461 [details] [diff] [review]
Manual fixes

(In reply to comment #4)
> rail, I think your two Manual Fixes patches are disjoint and we would need them
> both? am I wrong?

Oh, yeah. The result of staying too late. :)

Thanks for the catch.
Attachment #495425 - Attachment is obsolete: true
Attachment #495461 - Flags: review?(ted.mielczarek)
Attachment #495425 - Flags: review?(ted.mielczarek)
Comment on attachment 495461 [details] [diff] [review]
Manual fixes

>--- a/testing/testsuite-targets.mk
>+++ b/testing/testsuite-targets.mk
>@@ -48,17 +48,17 @@ endif
> SYMBOLS_PATH := --symbols-path=$(DIST)/crashreporter-symbols
> 
> # Usage: |make [TEST_PATH=...] [EXTRA_TEST_ARGS=...] mochitest*|.
> MOCHITESTS := mochitest-plain mochitest-chrome mochitest-a11y mochitest-ipcplugins
> mochitest:: $(MOCHITESTS)
> 
> RUN_MOCHITEST = \
> 	rm -f ./$@.log && \
>-	$(PYTHON) _tests/testing/mochitest/runtests.py --autorun --close-when-done \
>+	$(PYTHON) $(mochitestdir)/runtests.py --autorun --close-when-done \
> 	  --console-level=INFO --log-file=./$@.log --file-level=INFO \
> 	  $(SYMBOLS_PATH) $(TEST_PATH_ARG) $(EXTRA_TEST_ARGS)
> 
> ifndef NO_FAIL_ON_TEST_ERRORS
> define CHECK_TEST_ERROR
>   @errors=`grep "TEST-UNEXPECTED-" $@.log` ;\
>   if test "$$errors" ; then \
> 	  echo "$@ failed:"; \
>@@ -135,16 +135,17 @@ xpcshell-tests:
> 	  --manifest=$(DEPTH)/_tests/xpcshell/all-test-dirs.list \
> 	  --no-logfiles \
>           $(SYMBOLS_PATH) \
> 	  $(TEST_PATH_ARG) $(EXTRA_TEST_ARGS) \
> 	  $(DIST)/bin/xpcshell
> 
> # Package up the tests and test harnesses
> include $(topsrcdir)/toolkit/mozapps/installer/package-name.mk
>+include $(topsrcdir)/config/config.mk

This config.mk include seems unnecessary. (We clearly already use a bunch of stuff from config.mk in this file.)

Despite being a large scary invasive change, this seems ok.
Attachment #495461 - Flags: review?(ted.mielczarek) → review+
Attachment #495060 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #6)
> Despite being a large scary invasive change, this seems ok.

Ted Mielczarek, to resolve this bug we need only to delete that line from the patch?
This bug is preventing my build to work, so I would like to see it resolved.
Yes, with just that change we should be able to land this. Does this patch still apply to the latest source?
I can't build Firefox because of the same problem reported by Rail Aliiev, so I think it still applies.
This bug is also for Windows 7 64 bit, not only for Linux.
Created attachment 552904 [details] [diff] [review]
Manual fixes corrected

Removed the useless line.
Attachment #552904 - Flags: review?(ted.mielczarek)
Attachment #552904 - Flags: review?(ted.mielczarek) → review+
Attachment #495060 - Attachment is patch: false

Updated

6 years ago
Blocks: 682540
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
Attachment #495461 - Attachment is obsolete: true
Marco, there's something up with the patch as it doesn't import without a parse error into mercurial. I tried to fix it up, but it then still doesn't apply cleanly to tip - and manually resolving made me notice new instances (eg RUN_MOCHITEST_REMOTE) that need shortening now as well. 

Also, the diff of attached patch vs my fixed up patch is huge, which makes me a bit uneasy. The attached patch didn't have the full amount of context, which is probably the main reason why, but I don't have the time to pick through and check that something didn't break during fixing it up.

Please can you:
- check for new instances to shorten
- update to tip
- add author and commit message
- make sure the patch isn't malformed
- use the format/context settings here: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
- send to try
...and then re-request checkin-needed.

Thanks :-)
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Flags: in-testsuite-
Keywords: checkin-needed
Created attachment 567284 [details] [diff] [review]
Shorten mochitest paths for m-c, V4

I'm not sure if Rail/Marco was going to work on this but I happened to refresh the patch while I was introducing myself to mochitests. Patch is updated to tip and should handle current mochitests in m-c (there seems to have been a lot new tests created during last year!) as I run the attached shorteh.sh script and also verified that _tests/testing directory no longer contains anything (that's the old location) after a build. I tested running Windows mochitests locally and at least that started and run the tests through.

Additionally this patch removes the _mochitest directory at the same time _tests directory is removed for rebuilds. This way it should match the old behaviour and make sure automationutils.py and tests are refreshed without clobbering builds manually all the time.

I don't have try-server access so would be nice if somebody could run the patch through try for me, thanks!
Attachment #552904 - Attachment is obsolete: true
Attachment #567284 - Flags: review?(ted.mielczarek)
Pushed to try, but the specialpowers changes landed in the last day or so have bitrotted the patch in a few places. Did the best I could to fixed locally, but not sure if I got everything:
https://tbpl.mozilla.org/?tree=Try&rev=a69d2e0bfd28
Created attachment 567546 [details] [diff] [review]
Shorten mochitest paths for m-c, V4.1

Thanks for the try-run. I've fixed the one Moth failure that seems to have been due to two new DOM Worker test extensions makefiles and also otherwise unrotted this version.
Attachment #567284 - Attachment is obsolete: true
Attachment #567546 - Flags: review?(ted.mielczarek)
Attachment #567284 - Flags: review?(ted.mielczarek)
Comment on attachment 567546 [details] [diff] [review]
Shorten mochitest paths for m-c, V4.1

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

I didn't look at every single line of this patch, but I reviewed this in the past so I'm just going to r+ this. I did spot check a bunch of places and they look fine.
Attachment #567546 - Flags: review?(ted.mielczarek) → review+
Created attachment 567894 [details] [diff] [review]
Shorten mochitest paths for c-c, V4.1

This is the required changes in the comm-central repo.
Attachment #567894 - Flags: review?
Attachment #567894 - Flags: review? → review?(benjamin)

Updated

6 years ago
Attachment #567894 - Flags: review?(benjamin) → review+
Comment on attachment 567894 [details] [diff] [review]
Shorten mochitest paths for c-c, V4.1

If this was tested on c-c (not suite has horrid orange anyway) I give this patch a+ even if SeaMonkey tree is still closed, so we can avoid bitrot
Created attachment 570627 [details] [diff] [review]
Shorten mochitest paths for m-c, V4.2

Simple refresh due to recent code suffling. Carrying over r=ted since changes are minimal (or try to).
Attachment #567546 - Attachment is obsolete: true
Attachment #570627 - Flags: review+
Rail did all the heavy lifting here. I did run c-c suite tests locally and they did run with only few failures.
Assignee: mar.castelluccio → rail
Keywords: checkin-needed
(Reporter)

Comment 21

6 years ago
@ted: I have not enough privileges to land the patch to m-i/m-c. Could you land it?
Comment on attachment 570627 [details] [diff] [review]
Shorten mochitest paths for m-c, V4.2

This didn't apply cleanly to inbound, but I've fixed locally & added the missing author/commit message (for future patches, please see http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed).

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=9bcd060f8d1b

Thanks :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4997c6f8b24d
Keywords: checkin-needed
Target Milestone: --- → mozilla10
Created attachment 572084 [details] [diff] [review]
m-c part followup

dom/battery/test/Makefile.in was created since the initial patch, so needs changing too:
http://mxr.mozilla.org/mozilla-central/source/dom/battery/test/Makefile.in#59
Attachment #572084 - Flags: review?(ted.mielczarek)
Keywords: checkin-needed
Whiteboard: [checkin-needed for c-c] [m-c part: on merge, leave open]
https://hg.mozilla.org/mozilla-central/rev/4997c6f8b24d
I am having problems executing the mochitests:
http://pastebin.mozilla.org/1374664

If I do make -C ff-obj mochitest-browser-chrome it tells me firefox-bin doesn't exist.

Why does the script look for the firefox-bin in the src folder instead of the ff-obj directory?

I tried running the mochitests from the root src folder and from the ff-obj dir. Same error.
ditto what mihai said in comment #26.
why are we landing massive build system changes on the weekend before a merge?
Backed out preemptively in case this was the cause (and yet works on try/m-c somehow), given that if it is, it's stopping people test locally leading up to the merge:

https://hg.mozilla.org/mozilla-central/rev/e697147121b3
Keywords: checkin-needed
Whiteboard: [checkin-needed for c-c] [m-c part: on merge, leave open]
Target Milestone: mozilla10 → ---
Comment on attachment 572084 [details] [diff] [review]
m-c part followup

Is obsolete, as will be added to the main patch when it relands.
Attachment #572084 - Attachment is obsolete: true
Attachment #572084 - Flags: review?(ted.mielczarek)
confirmed that this was the cause of the problem.

Please wait until after Tuesday (November 8th) before relanding this. We can deal with breakage then.

thanks.
Target Milestone: --- → mozilla10
Target Milestone: mozilla10 → ---
Thanks for confirming Rob, I'll leave it to Rail/wicked to fix, test locally and re-request review from ted.
(Reporter)

Comment 33

6 years ago
I'm not sure if I'm the best person to be an assignee. :( Back to the pool of Core/Build Config.
Assignee: rail → nobody
Status: ASSIGNED → NEW
Rail, I assigned this to you to mark the fact that you did the initial/most work (and thus should get credit for fixing this, not sure if you follow that principle, though) but I guess that'll work better when this is actually fixed. :) Anyway, I'll update the patch as soon as I figure out what's wrong.
Assignee: nobody → wicked
Status: NEW → ASSIGNED
(Reporter)

Comment 35

6 years ago
Teemu, I just wanted to keep my "Assigned bugs" list shorter :) and make sure that Core/Build Config "unassigned bugs" (tirage?) query lists this bug. Thanks a lot for your hard work keeping the patch up to date and testing it!
Actually, I doubt I'll get to this ever but this seems to be taken care of slowly by other bugs anyway..
Assignee: wicked → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.