Closed Bug 1089037 Opened 10 years ago Closed 10 years ago

Invalid mochitests skipped in webapprt/test/content/mochitest.ini

Categories

(Core :: General, defect)

defect
Not set
normal

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
I thought we fixed this already, although I am not 100% sure.
Attached patch webapprt.patch (obsolete) — Splinter Review
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)
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.
Attached patch webapprt.patch (obsolete) — Splinter Review
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
Attached patch webapprt.patchSplinter Review
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: nobody → vaibhavmagarwal
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+
Keywords: checkin-needed
Keywords: checkin-needed
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+
I think it is ok to have the filename ("webapprt.ini") since they are webapprt tests. Thanks for reviewing this!
Keywords: checkin-needed
No build peer review?
https://hg.mozilla.org/mozilla-central/rev/4d37b7dac785
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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 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 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+
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.