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

Some assertions generate always changing messages when failing #1984

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Jul 10, 2015

When failing, some assertions dump token values which always change test run after test run.
This defeats WebKit test infrastructure which is checking text output against expected text output.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/5507

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jgraham
Copy link
Contributor

jgraham commented Jul 10, 2015

Constant assertion text is explicitly not part of the contract that th.js users are supposed to conform to (indeed the harness itself fills variables into the failure message). What must be constant is the title of each test.

It seems like this is a bug in the WebKit integration.

hallvors added a commit that referenced this pull request Jul 10, 2015
…ertions

Some assertions generate always changing messages when failing
@hallvors hallvors merged commit ac3ac82 into web-platform-tests:master Jul 10, 2015
@hallvors
Copy link
Contributor

I can see some value in how WebKit's framework is operating - it certainly simplifies regression testing to have an "expected" output for all tests - and it's not a big issue to take in these few changes, so I just merged them. However there's probably going to be a lot more of this and @jgraham is right - testharness.js aims to be human user friendly and show at a glance the expected and the given value, so it might be better to have an intermediate step before you save the results in your framework, processing the results.

@youennf
Copy link
Contributor Author

youennf commented Jul 10, 2015

@jgraham, I do not disagree (on the contrary) that WebKit infrastructure could be made more aligned with WPT infrastructure. But consensus on that is not reached within WebKit community and progress is made slow.
In the meantime, minor modifications like these ones are helpful.
Thanks for merging it!

@jgraham
Copy link
Contributor

jgraham commented Jul 10, 2015

I don't object to this being merged, but I will certainly object if you go through the whole of wpt and try to make the same changes.

My suggestion is that you either collect results in an entirely different way for testharness.js tests compared to other layout tests or implement a testharnessreport.js that produces output only from the tests and not the individual asserts.

If you want me to explain the testharness situation to the rest of the WebKit community please CC me on any relevant mailing list threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants