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

Run our reftests with wptrunner #6898

Closed
wants to merge 5 commits into from
Closed

Run our reftests with wptrunner #6898

wants to merge 5 commits into from

Conversation

@sgmenda
Copy link
Contributor

sgmenda commented Aug 2, 2015

#5618, It allows us to run reftests using wptrunner

Review on Reviewable

boghison and others added 4 commits Jul 23, 2015
32-bit floats are not enough to hold timestamps since the epoch and
result in jank.
s142857
Ability to run reftests using wptrunner
@highfive
Copy link

highfive commented Aug 2, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon.

@jdm
Copy link
Member

jdm commented Aug 2, 2015

This is a great effort! However, there's an important piece missing - the addition of the <link> that tells the test harness which file to compare against. This is also why I don't see any changes to MANIFEST.json, I think. Also, the output would be much easier to read if we let git know about the file moves with git mv, rather than copying them to the new folder. Does that make sense?

@sgmenda
Copy link
Contributor Author

sgmenda commented Aug 2, 2015

yup,
How do I add the code (link)
How should I update the manifest.json
I have no idea about git mv, could you help me

Is it worth for someone like me to try to contribute to servo???

@jdm
Copy link
Member

jdm commented Aug 2, 2015

If you're not afraid of spending time to learn lots of new things, I think it should be fine :)

MANIFEST.json is automatically updated by running the test-wpt mach command with --manifest-update. However, it will only update the file if it sees new tests that include changes like this - does that make sense?

As for using git mv, does this page answer your question?

@sgmenda
Copy link
Contributor Author

sgmenda commented Aug 2, 2015

the github link is broken

@sgmenda
Copy link
Contributor Author

sgmenda commented Aug 2, 2015

As for the move, i used git add instead for the time being to ensure backwards compatibility, if it wont cause any issues i will remove it from ref

@jdm
Copy link
Member

jdm commented Aug 2, 2015

It's a lot easier to review the changes if git mv is used :) The github link is fixed now, thanks.

@sgmenda
Copy link
Contributor Author

sgmenda commented Aug 2, 2015

but what if someone wanted to run the ref tests the old way,

PS the link still dosent work
https://github.com/servo/servo/pull/5687/files#diff-b7983c29eb43fd95994bf5386665018aR4

@jdm
Copy link
Member

jdm commented Aug 2, 2015

The point of this change is that the old way is strictly worse than the new way, so we want to convert all of the tests to the new way. As for the link problem, the one in my comment and your link both work for me :/

@sgmenda
Copy link
Contributor Author

sgmenda commented Aug 2, 2015

ok you want me to reset and use git mv or
can I use git rm

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.