Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Collapse mozilla-central Ahem lint exception to one line" #18929

Closed
wants to merge 1 commit into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Sep 9, 2019

See #18877 (comment)

Reverts #18901

@foolip
Copy link
Member Author

foolip commented Sep 9, 2019

@dbaron @jgraham do with this as you will :)

@dbaron
Copy link
Member

dbaron commented Sep 9, 2019

I think we're probably better off with this as they are (one line), since as far as I can tell, it's not possible for a test in that directory to pass this particular lint rule since the absolute file paths required wouldn't work for running in both reftest harnesses.

@jgraham
Copy link
Contributor

jgraham commented Sep 18, 2019

However I think we should stop adding tests to that directory, so this stops being a problem. Going forward, if we are adding tests that depend on system Ahem and there are configurations where Ahem isn't present on the system because wpt no longer considers that a requirement, these tests will get broken anyway, so I think the general lint rule (and indeed having exceptions) is just going to hide a real problem.

@dbaron
Copy link
Member

dbaron commented Sep 18, 2019

The issue here isn't that tests are depending on system Ahem; they're just loading the Ahem webfont at a different path and not the canonical path for the rest of WPT.

@foolip
Copy link
Member Author

foolip commented Sep 18, 2019

@jgraham does that clarify such that it's fine to merge this? You approved, but then there was discussion :)

@jgraham
Copy link
Contributor

jgraham commented Sep 23, 2019

Per @dbaron's previous comment it sounds like the correct way forward is to fix the lint to allow what gecko's doing and remove all the exceptions related to this issue.

@jgraham
Copy link
Contributor

jgraham commented Sep 23, 2019

#19218 for that change. I'm going to close this one for now.

@jgraham jgraham closed this Sep 23, 2019
@foolip foolip deleted the revert-18901-foolip/m-c-ahem-lint branch September 23, 2019 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants