Skip to content

Conversation

@Cactusmachete
Copy link
Contributor

@Cactusmachete 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

…s to other type

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

Closes web-platform-tests#9671
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ghost
Copy link

ghost 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 cactus branch March 1, 2018 11:42
@gsnedders
Copy link
Member

This turned into #9732.

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.

4 participants