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

manifest.py: Fixed updating reftest graph when node changes to other type #9719

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@Cactusmachete
Copy link
Contributor

Cactusmachete commented Feb 28, 2018

Slightly altered line 94 so reftest_changes is also set when
reftest_node changes to support, etc.

Closes #9671


This change is Reviewable

Cactusmachete
manifest.py: Fixed updating of reftest graph when reftest_node change…
…s to other type

Slightly altered line 94 so reftest_changes is also set when
reftest_node changes to support, etc.

Closes #9671

@wpt-pr-bot wpt-pr-bot added the infra label Feb 28, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Feb 28, 2018

@jgraham
Copy link
Contributor

jgraham left a comment

I'm pretty sure the logic here is correct, thanks! Could you add a test in tools/manifest/tests/test_manifest.py to check that this doesn't regress?

@@ -91,7 +91,7 @@ def update(self, tree):
hash_changed = True
else:
new_type, manifest_items = old_type, self._data[old_type][rel_path]
if old_type == "reftest" and new_type != old_type:
if (old_type == "reftest" and new_type != old_type) or (old_type=="reftest_node" and new_type!=old_type):

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 28, 2018

Contributor

I think this can be written old_type in ["reftest", "reftest_node"] and new_type != old_type.

This comment has been minimized.

Copy link
@Cactusmachete

Cactusmachete Feb 28, 2018

Author Contributor

Sorry, didn't think about that earlier. Made that change now.
I'm not sure about how to write tests so I'll have to look into that

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 28, 2018

Contributor

It'd be more idiomatic to use a tuple (("reftest", "reftest_node")) here.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 28, 2018

Build PASSED

Started: 2018-02-28 18:15:16
Finished: 2018-02-28 18:31:02

View more information about this build on:

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

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Mar 2, 2018

This turned into #9732.

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.