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

add ASTextNode2 snapshot test #935

Merged

Conversation

wsdwsd0829
Copy link
Contributor

Just copy over from ASTextNode and enable experiment.
// All tests are duplicated from ASTextNodeSnapshotTests.

@ghost
Copy link

ghost commented May 23, 2018

🚫 CI failed with log

Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

LGTM, but please check the failing tests. Thanks!

@wsdwsd0829
Copy link
Contributor Author

Ok, I am a little confused, the tests seems so comparing images from TextNode1.
From what I recorded it locally, it supposed to create a new folder with name of this new test class.

Will try enable record mode and rely on CI to create snapshots itself and then disable it, hopefully it can fix CI fails.

@wsdwsd0829
Copy link
Contributor Author

May needs some suggestions on this. It's seems comparing with TextNode1's images.

@TextureGroup TextureGroup deleted a comment May 26, 2018
@TextureGroup TextureGroup deleted a comment May 26, 2018
@Adlai-Holler
Copy link
Member

Adlai-Holler commented May 26, 2018

It appears that FBSnapshotTestCase will use whatever reference image it matching the test selector that it finds first.

The code in - (NSString *)_referenceFilePathForSelector:(SEL)selector identifier:(NSString *)identifier doesn't know what class the selector is on.

The easiest fix is to rename the methods to e.g. testTextContainerInset2 and put a comment at the top explaining why we did and linking to this PR.

@ghost
Copy link

ghost commented May 29, 2018

🚫 CI failed with log

@TextureGroup TextureGroup deleted a comment May 29, 2018
@Adlai-Holler
Copy link
Member

@wsdwsd0829 It looks like the issue is that the ASTextNode1 tests are running after the ASTextNode2 tests, and there's currently no way to go back to ASTextNode1 once you enable ASTextNode2.

So the TextNode1 tests are running with TextNode2 enabled.

@wsdwsd0829
Copy link
Contributor Author

wsdwsd0829 commented May 29, 2018 via email

@Adlai-Holler
Copy link
Member

@wsdwsd0829 We're working on getting the CI back up and running.

@wsdwsd0829
Copy link
Contributor Author

Thanks for suggestions and fixing this.

@Adlai-Holler
Copy link
Member

Very great diff! This will let us iterate on ASTextNode2 safely ❤️

@Adlai-Holler Adlai-Holler merged commit 9fe2f47 into TextureGroup:master Jun 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants