1848994 - for new .toml files allow for comments to be on the same line with skip-if condition

Status: RESOLVED FIXED

(Testing :: Mozbase, task, P2)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

preserving comments is important for our manifests, it keeps history and state easy to find. as we switch to .toml, this can get confusing... from Matrix:

I ran into one issue that I'm sure you're aware of. I noticed you're often ending the triple quote for skip-if on the same line as the condition, e.g:

['test_docload_shutdown.html']
skip-if = '''
  os == 'mac' '''  # bug 1456997

That makes sense so we know the bug is associated with that condition. But if you add another condition, it stops working:

['test_docload_shutdown.html']
skip-if = '''
  os == 'mac'  # bug 1456997
  display == 'wayland'  # bug 1234567
'''

as those are no longer comments and are part of the string.

I know you wanted to keep things working the same in manifestparser across both formats, but I'm thinking we should convert this to a list and special case toml in manifestparser :/. E.g, so we'd have:

['test_docload_shutdown.html']
skip-if = [
  "os == 'mac'",   # bug 1456997
  "display == 'wayland'",  # bug 1234567
]

I don't think it would be a huge change in manifestparser, something like:

if path.endswith("toml"):
    skip_if = obj["skip-if"]
else:
    skip_if = obj["skip-if"].split("\n")

expression  = " || ".join(skip_if)

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

:tmarble, I will let you figure out when and how to fix this, the comments here are a realistic solution.

Comment 2

[Tom Marble]

Based on discussion with :ahal in Firefox Testing we have decided the approach to this bug is:

1. Fixi the currently migrated TOML files with skip-if to a list
2. Update mmp to translate skip-if to a list
3. Verify that the manifestparser TOML reader will "do the right thing" (handle implicit OR as newlines and ignore comments)

Comment 3

[Tom Marble]

Attachment: Bug 1848994 Allow comments in manifestparser conditions. r=jmaher

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d5cf6ae2b0b
Allow comments in manifestparser conditions. r=jmaher

Comment 5

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/9d5cf6ae2b0b