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

Update reftest graph when node changes to support #9732

Merged
merged 2 commits into from Mar 1, 2018

Conversation

Projects
None yet
4 participants
@Cactusmachete
Copy link
Contributor

Cactusmachete commented Mar 1, 2018

Modified conditional in manifest.py to check for a reftest node changing
to another type.
Wrote a test in test_manifest.py for the same, but this needs some review.

Closes #9671


This change is Reviewable

Cactusmachete
Update reftest graph when node changes to support
Modified conditional in manifest.py to check for a reftest node changing
to another type.
Wrote a test in test_manifest.py for the same, but this needs some review.

Closes #9671

@wpt-pr-bot wpt-pr-bot added the infra label Mar 1, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Mar 1, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 1, 2018

Build PASSED

Started: 2018-03-01 13:30:40
Finished: 2018-03-01 13:41:34

View more information about this build on:

@jgraham
Copy link
Contributor

jgraham left a comment

I think the way you've written the test effetively creates an entirely new manifest that's not related to the previous one.

Note that in general one should push additional commits to the existing PR (by pushing to the same branch) rather than creating a new PR with changes.

# This doesn't check whether the reftest graph was computed or not,
# the m.update([s3]) assert will just check whether m.update() returned True (I think).
# Regardless, the test works even when the graph isn't being recomputed.
assert m.update([s3]) is True

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 1, 2018

Contributor

Then here I think you want to call m.update([s1, s2]) again.

("reftest_node", test2.path, {test2})]

#test2 changes to support type
s3 = SourceFileWithTest("test2", "1"*40, item.SupportFile)

This comment has been minimized.

Copy link
@jgraham

jgraham Mar 1, 2018

Contributor

So I think you want to redefine s2 here rather than creating a new s3

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Mar 1, 2018

Author Contributor

I made the changes and it works as it should now!
Still need to understand why, though.
Thanks a lot 😄

And I'm sorry I made a new PR for this, I'll ensure I push to the same branch from now on.

Cactusmachete
@jgraham

jgraham approved these changes Mar 1, 2018

Copy link
Contributor

jgraham left a comment

Thanks!

@jgraham jgraham merged commit 2560d71 into web-platform-tests:master Mar 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Cactusmachete Cactusmachete deleted the Cactusmachete:cactus branch Mar 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.