|
| 1 | +# RFC 158: Variant name should be a non-zero length string |
| 2 | + |
| 3 | +## Summary |
| 4 | + |
| 5 | +Make a policy change to disallow zero length Variant names. |
| 6 | + |
| 7 | +## Details |
| 8 | + |
| 9 | +A test file can have multiple variants by including meta elements. |
| 10 | +Even though zero-length variant names are allowed currently, we |
| 11 | +can not run it the same way as other variants. |
| 12 | + |
| 13 | +For example, the command below runs `websockets/binary/005.html?wss` |
| 14 | +only. |
| 15 | +``` |
| 16 | +wpt run ... websockets/binary/005.html?wss |
| 17 | +``` |
| 18 | + |
| 19 | +But the commands below run all three variants in the file. |
| 20 | +``` |
| 21 | +wpt run ... websockets/binary/005.html |
| 22 | +wpt run ... websockets/binary/005.html? |
| 23 | +``` |
| 24 | + |
| 25 | +This consequently causes problem when we do external sharding, then |
| 26 | +pass all the tests we want to run to wptrunner through `--include`. |
| 27 | +While we only intended to run the empty string variant by passing |
| 28 | +`websockets/binary/005.html`, wptrunner runs all the variants in |
| 29 | +that file, causing the non-empty variants to run twice. It is |
| 30 | +possible we shard all the variants from the same file to the same |
| 31 | +shard, this could load the different shards unevenly, and increase |
| 32 | +the perceived total run time. |
| 33 | + |
| 34 | +Proposal is to disallow zero length variant names. |
| 35 | + |
| 36 | +### Implementations |
| 37 | + |
| 38 | +Below are the changes required to make this happen: |
| 39 | +1. Update all the existing tests that have an empty variant name. Update |
| 40 | +the supporting code if necessary to make sure the tests continue to |
| 41 | +work. |
| 42 | +2. Add a lint to `wpt lint` to make it an error for zero length variant |
| 43 | +names. |
| 44 | +3. Update the document to not use zero length variant names as examples. |
| 45 | +4. Update manifest generation code to throw an error for empty variant |
| 46 | +names. |
| 47 | + |
| 48 | +### Alternatives Considered |
| 49 | + |
| 50 | +It is possible we update wptrunner to accept `path/to/test.html?` as the |
| 51 | +url for the empty name variants, but such changes would be hard to implement |
| 52 | +and it will be surprising to the developers. Meanwhile making people name |
| 53 | +each variant does add a little extra documentation. |
| 54 | + |
| 55 | +Alternatively we can add `/` at the beginning of a test name, to denote |
| 56 | +the parameter is an url instead of a path. This deviates further from |
| 57 | +how developers are running tests now, and it will break the command line |
| 58 | +auto-completion feature. |
| 59 | + |
| 60 | +## Risks |
| 61 | + |
| 62 | +As this is adding restrictions to the existing system, it should not break |
| 63 | +anything. |
| 64 | + |
| 65 | +Developers should be able to run the tests the same way as before, with |
| 66 | +the added capability to run the previous empty name variant alone. |
0 commit comments