Closed
Bug 1089037
Opened 10 years ago
Closed 10 years ago
Invalid mochitests skipped in webapprt/test/content/mochitest.ini
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jgilbert, Assigned: vaibhav1994)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
> 16:07:16 INFO - Warning: webapprt_sample.html from manifest /builds/slave/test/build/tests/mochitest/tests/webapprt/test/content/mochitest.ini is not a valid test
> 16:07:16 INFO - Warning: webapprt_indexeddb.html from manifest /builds/slave/test/build/tests/mochitest/tests/webapprt/test/content/mochitest.ini is not a valid test
Comment 1•10 years ago
|
||
I thought we fixed this already, although I am not 100% sure.
Assignee | ||
Comment 2•10 years ago
|
||
The patch adds webapprt tests as support files. :billm, is this the proper way to go about it because they are skipped by mochitest. I do not know how to test webapprt tests.
Flags: needinfo?(wmccloskey)
Comment on attachment 8518199 [details] [diff] [review] webapprt.patch Review of attachment 8518199 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this is right. If you make this change, the tests will no longer run. You can run the webapprt tests by doing: mach webapprt-content or mach webapprt-chrome (There are two separate test suites.) I agree that something needs to be done about this, but I'm not sure what. Marco Castellucci is the owner of these tests as far as I know. Perhaps you guys could discuss it here.
Attachment #8518199 -
Flags: review-
Flags: needinfo?(wmccloskey) → needinfo?(mar.castelluccio)
Comment 4•10 years ago
|
||
The tests are run via the "webapprt-test-content" mach command. I don't have any experience with how the tests are run via the mochitest framework. My experience is limited to the tests themselves. Maybe one way to fix this is by having a different name for the webapprt-test-content INI file (just like we did with webapprt-test-chrome, where we used webapprt.ini instead of chrome.ini).
Flags: needinfo?(mar.castelluccio)
Yes, I think this would be a good way to go.
Assignee | ||
Comment 6•10 years ago
|
||
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e21267ef4324 The webapprt content tests works for me with this patch. I have pushed to try to verify that warnings for webapprt-content are indeed not present anymore.
Attachment #8518199 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
The try run is as expected with green jobs and no more warnings related to webapprt content tests. :jmaher and :marco please review this patch. I have moved the webapprt content tests to a separate "webapprtContent" folder like "webapprtChrome" folder in object directory.
Attachment #8522413 -
Attachment is obsolete: true
Attachment #8522451 -
Flags: review?(mar.castelluccio)
Attachment #8522451 -
Flags: review?(jmaher)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → vaibhavmagarwal
Comment 8•10 years ago
|
||
Comment on attachment 8522451 [details] [diff] [review] webapprt.patch Review of attachment 8522451 [details] [diff] [review]: ----------------------------------------------------------------- thanks for making this!
Attachment #8522451 -
Flags: review?(jmaher) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Comment on attachment 8522451 [details] [diff] [review] webapprt.patch Review of attachment 8522451 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! Should we have different names for the INI files related to webapprt-test-chrome and webapprt-test-content, or is it ok to have the same filename?
Attachment #8522451 -
Flags: review?(mar.castelluccio) → review+
Assignee | ||
Comment 10•10 years ago
|
||
I think it is ok to have the filename ("webapprt.ini") since they are webapprt tests. Thanks for reviewing this!
Keywords: checkin-needed
Comment 11•10 years ago
|
||
No build peer review?
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d37b7dac785
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4d37b7dac785
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8522451 [details] [diff] [review] webapprt.patch This code has landed, however on :Ms2ger's recommendation, adding Michael as the reviewer. Michael, please review this and let me know if I need to do any changes. Thanks
Attachment #8522451 -
Flags: review?(mshal)
Comment 15•10 years ago
|
||
Comment on attachment 8522451 [details] [diff] [review] webapprt.patch To me it feels wrong to introduce MOCHITEST_WEBAPPRT_CONTENT_MANIFESTS when it's used in only one file. Do you expect to use it in many other places in the future? In any case, I'll let gps weigh in here.
Attachment #8522451 -
Flags: review?(mshal) → review?(gps)
Comment 16•10 years ago
|
||
Comment on attachment 8522451 [details] [diff] [review] webapprt.patch Review of attachment 8522451 [details] [diff] [review]: ----------------------------------------------------------------- It does feel weird for this to be a one-off. I think a better solution for small "suites" like this would be to add a flag to the test manifest file - "content = true" or some such - and apply a filter or appropriate behavior at the test harness layer. But the cat is out of the bag. I'll grant r+. We can always undo this in a follow-up if it becomes annoying to support.
Attachment #8522451 -
Flags: review?(gps) → review+
Comment 17•7 years ago
|
||
Moving from Core::Untriaged to Core::General https://bugzilla.mozilla.org/show_bug.cgi?id=1407598
Component: Untriaged → General
You need to log in
before you can comment on or make changes to this bug.
Description
•