Closed Bug 574189 Opened 10 years ago Closed 10 years ago
fix chrome style tests which fail when run from a .jar package
in bug 543800, we are packaging all the chrome/browser-chrome/a11y tests into a .jar package. I have found that there are 45 mochitest chrome tests and 2 browser-chrome tests that fail when run this way.
Comment on attachment 453560 [details] [diff] [review] fix chrome tests (v 0.1) Laurent, Can you look at the storage tests here and see if changing the datasource from animals.sqlite to profile:animals.sqlite will achieve the same test coverage.
Attachment #453560 - Flags: feedback?(laurent)
Comment on attachment 453560 [details] [diff] [review] fix chrome tests (v 0.1) I didn't test but it seems good for me. However, perhaps it could be a good thing to move the copyToProfile function to Mochitest or SimpleTest library (I don't know where exactly). It could be useful for other tests.
Attachment #453560 - Flags: feedback?(laurent) → feedback+
updated patch to fix the toolkit/content/tests/chrome/* tests, specifically the rtltest/ and rtlchrome/ testcases. This is not a final patch, as I plan to clean up the utility functions I am using to extract and find files in the .jar files. :ehsan, can you review this to assure my workaround for test_bug437844.xul and test_righttoleft.xul are actually testing the right things and you are happy with the solution.
Why have you put the guts of that test in a window? Also, I think I need to test this in a try server build, have you pushed one?
forgot to update all changes to test_righttoleft.xul. Kicking off a try server run, local run on desktop linux passed when these tests were failing before.
Hmm, do the try server builds include a test package? If not, I could also verify if these tests are testing the correct thing if you do a screen record while running the tests, and send me the video! :-)
I am going to push my .jar package and test fix patches at the same time.
updated with a lot of tests and passing on try server.
Attachment #460222 - Flags: review?(ctalbert) → review-
updated with review feedback. Thanks for all the great suggestions! Tested locally, currently running on try server.
updated for bitrot in 1 file.
Comment on attachment 462293 [details] [diff] [review] fix chrome tests (v 2.1) ctalbert: jmaher: I thought you were going to change all the references of mochitest harness stuff to chrome:// paths? [7:12pm] ctalbert: and all things not harness stuff was not going to be loaded with relative paths [7:12pm] ctalbert: er and all things not harness stuff was not going to be loaded with relative paths [7:13pm] jmaher: ctalbert: yeah [7:13pm] ctalbert: bear, huh, have to check on that one then. We basically kick off the harness and let it go, so it's likely something in that test interacting poorly with either the standard reftest harness in chrome or that bit not working with fennec [7:14pm] jmaher: that patch should be exactly what you said [7:14pm] ctalbert: jmaher: look at the diff on that bug -- third diff down, test_draggableprop.html - exactly the opposite [7:14pm] ctalbert: you're changing mochitest harness stuff to use local paths [7:14pm] ctalbert: er relative paths [7:14pm] ctalbert: damn but I can't type [7:16pm] ctalbert: jmaher: since we wanted to keep the chrome paths for mochitest harness stuff, that seems like unnecessary change [7:17pm] jmaher: ctalbert: I removed the chrome:// from mochitest-plain [7:17pm] • ctalbert looks again [7:17pm] jmaher: because that requires us to have the mochikit.* installed [7:18pm] ctalbert: oh shit those are mochitest-plain tests [7:18pm] jmaher: so those tests that have it removed are just mochitest-plain [7:18pm] jmaher: sorry, I should have been a bit more clear [7:18pm] ctalbert: ok, cool [7:18pm] ctalbert: very very good, then. well done jmaher [7:18pm] jmaher: heh, thanks! [7:18pm] • ctalbert starts again at the top [7:19pm] • jmaher passed the first quest [7:19pm] • ctalbert was somehow thinking all the tests in this patch were chrome tests. [7:21pm] ctalbert: jmaher: if the stuff in content/xul/templates/tests/chrome/templates_shared.js throws, then the test is going to fail, correct? [7:21pm] jmaher: heh, this patch cleans up a lot of stuff [7:22pm] ctalbert: i.e. if copyToProfile throws, then the test is going to fail as it won't have its files available [7:22pm] jmaher: correct [7:22pm] ctalbert: So, can we do an ok(false, "Cannot copy files to profile due to: " + ex); in that catch statement just to aid in debugging later (when they change e10s again) [7:23pm] ctalbert: [7:23pm] jmaher: yeah! [7:23pm] jmaher: good idea [7:28pm] ctalbert: I'd check dom/tests/mochitest/whatwg/test_postMessage_chrome.html -- looks like you're removing the beginning of a comment but not the end of it? [7:29pm] ctalbert: not sure why you're removing the comment in teh first place, but that coudl be a bugzilla viewer bug [7:33pm] jmaher: ctalbert: hmm, the diff is a bit odd, but yeah [7:34pm] jmaher: the diff is --->, where --> is the termination of the comment and the extra - is the diff minus [7:35pm] ctalbert: ah, I figured it was soemthing like that, which probably only appears in the raw view of the diff [7:35pm] jmaher: why the comment is removed? I believe I did it because it was either a mochikit.jar!/path, or a relative path...not what the old comment used to be [7:37pm] ctalbert: I figured, ok [7:37pm] ctalbert: I was thinking it would still be served from chrome, but yet the exact url would have to change coming from a jar [7:39pm] ctalbert: jmaher: one more -- down a ways in test_syntesizeDrop.xul - you remove utils.js? Not sure what's in there...going to look that up [7:39pm] ctalbert: I see why, it's one of those horrible cross-links [7:40pm] • ctalbert just wonders how the test keeps functioning [7:40pm] jmaher: ctalbert: I don't believe it was being used [7:40pm] jmaher: there is still one more test in the tree that cross references a library [7:40pm] jmaher: but in this case it was included but not used [7:40pm] ctalbert: jmaher: awesome catch [7:42pm] jmaher: ctalbert: good catch on your part with the detailed review [7:43pm] ctalbert: haha, I try [7:44pm] ctalbert: jmaher: ok, last one review comment - in chrome-harness.js - it's no longer 2008, so update your license header. [7:45pm] • jmaher is living in the past [7:45pm] ctalbert: r+ with those changes
Attachment #462293 - Flags: review?(ctalbert) → review+
updated with nit's from r+
Joel, I watched the tree all day moving from half/closed to burning to closed, and was unable to land this for you. I'm setting to checkin-needed, perhaps you can get Ted or Jgriffin to land this for you next week. Sorry about that.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Slight update to error messages for copyToProfile where it would spew errors when the file existed (if a second test copied to the same location). This was causing a lot of errors on windows runs and I had it locally but not on this patch. This passed twice on try server over the weekend.
Attachment #463043 - Attachment is obsolete: true
this was backed out shortly thereafter as a result of a series of failures with the whole set of patches that landed with it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #17) > Created attachment 468287 [details] [diff] [review] > fix chrome tests (v 2.3) > > Slight update to error messages for copyToProfile where it would spew errors > when the file existed (if a second test copied to the same location). This was > causing a lot of errors on windows runs and I had it locally but not on this > patch. > > This passed twice on try server over the weekend. Landed this version: http://hg.mozilla.org/mozilla-central/rev/a90640c20652
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.