| Summary: | [ews-build.webkit.org] Add unit test with the expected build steps for each queue | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> | ||||||
| Component: | Tools / Tests | Assignee: | Carlos Alberto Lopez Perez <clopez> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aakash_jain, ap, clopez, jbedard, mcatanzaro, pnormand, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=229311 | ||||||||
| Attachments: |
|
||||||||
|
Description
Carlos Alberto Lopez Perez
2021-08-19 18:03:57 PDT
Created attachment 435931 [details]
Patch
Comment on attachment 435931 [details]
Patch
This was fast!
Should we delete all the existing unit-test in this file (since each queue/factory would now be covered by two separate unit-tests, and any change in a factory would require updating two similar unit-tests), or do you see any value in retaining them?
(In reply to Aakash Jain from comment #2) > Comment on attachment 435931 [details] > Patch > > This was fast! > > Should we delete all the existing unit-test in this file (since each > queue/factory would now be covered by two separate unit-tests, and any > change in a factory would require updating two similar unit-tests), or do > you see any value in retaining them? I have doubts regarding that so I took the more conservative approach. It seems the test functionality is kind of duplicated now, so I guess that deleting the previous ones makes sense to avoid having to maintain the two Created attachment 435940 [details]
Patch
Comment on attachment 435940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435940&action=review > Tools/CISupport/ews-build/factories_unittest.py:32 > + "Style-EWS": [ Nit: we prefer using single quotes, but might be ok for now, we can change it later as well. > Tools/CISupport/ews-build/factories_unittest.py:572 > + self.assertListEqual(self.expected_steps[builder['name']], buildSteps, msg="Expected results don't match for builder %s" % builder['name']) Nit: "results" => "steps" (In reply to Aakash Jain from comment #5) > Comment on attachment 435940 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435940&action=review > > > Tools/CISupport/ews-build/factories_unittest.py:32 > > + "Style-EWS": [ > > Nit: we prefer using single quotes, but might be ok for now, we can change > it later as well. > > > Tools/CISupport/ews-build/factories_unittest.py:572 > > + self.assertListEqual(self.expected_steps[builder['name']], buildSteps, msg="Expected results don't match for builder %s" % builder['name']) > > Nit: "results" => "steps" Ok. Changing that here and also on the equivalent test for build.webkit.org for consistency Committed r281322 (240739@main): <https://commits.webkit.org/240739@main> |