Skip to content
This repository was archived by the owner on Oct 26, 2018. It is now read-only.

Conversation

@kimjoar
Copy link
Collaborator

@kimjoar kimjoar commented Dec 5, 2015

I think this nicely cleaned up a couple of the tests. Plus it's easier to not specify the changeId while still keeping the tests easily readable.

@ellbee
Copy link
Member

ellbee commented Dec 5, 2015

Very nice! 👍

Copy link
Member

Choose a reason for hiding this comment

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

I think I like this. changeId is a case where it might be too much implementation detail to encode in tests.

This is one place where we lose information though. You aren't testing that history.pushState is actually called twice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good catch, @jlongster. I'll fix asap.

@kimjoar
Copy link
Collaborator Author

kimjoar commented Dec 5, 2015

@jlongster This should cover it, I think.

@jlongster
Copy link
Member

Cool. (I was wrong in that the place that actually tested it was later in the file)

jlongster added a commit that referenced this pull request Dec 5, 2015
Create test helper to improve tests
@jlongster jlongster merged commit a7cf7a7 into reactjs:master Dec 5, 2015
@jlongster
Copy link
Member

I promise to look at the other PRs and resolve the devtools issues today or tomorrow!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants