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

Reducer specs #150

Closed
wants to merge 4 commits into from
Closed

Reducer specs #150

wants to merge 4 commits into from

Conversation

diogobeda
Copy link

@diogobeda diogobeda commented Jul 16, 2016

Hi there, I wanted to write some tests for the app and I wanted to start by the reducers. On this first commit, I only configured the tests environment/command and wrote a sample spec so you can give your opinions on both. After we agree on a standard, I'll write the rest of the tests using them.

What do you think?

cc @rauchg

@diogobeda diogobeda changed the title Reducer specs [Do not merge] Reducer specs Jul 16, 2016
"eslint-config-standard": "5.3.1",
"eslint-plugin-promise": "1.3.2",
"eslint-plugin-react": "5.2.2",
"eslint-plugin-standard": "1.3.2",
"mocha": "^2.5.3",
Copy link
Member

Choose a reason for hiding this comment

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

I prefer ava these days:
https://github.com/avajs/ava

Copy link
Author

Choose a reason for hiding this comment

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

sweet! gonna take a look at it

@rauchg
Copy link
Member

rauchg commented Jul 17, 2016

Looks awesome so far. Thank you for taking on such an important thing.

@diogobeda diogobeda changed the title [Do not merge] Reducer specs Reducer specs Jul 18, 2016
@diogobeda
Copy link
Author

@rauchg I think we're all set with reducer specs. From now on, I intend to write tests to other things as well.

@@ -22,17 +22,22 @@
"seamless-immutable": "6.1.1"
},
"devDependencies": {
"ava": "^0.15.2",
"ava-spec": "^1.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Can you please pin deps? Thank you!

Copy link
Author

Choose a reason for hiding this comment

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

sure! will do :)

@rauchg
Copy link
Member

rauchg commented Jul 19, 2016

Pinging @sindresorhus in case you wanna give the ava-ness of this a look :D

@sindresorhus
Copy link
Contributor

Generally looks good, although I'm personally not a big fan of all the describe/it nesting.

app/spec/reducers/sessions.spec.js

Feels moot adding .spec when the files are already in a spec directory.

@rauchg
Copy link
Member

rauchg commented Jul 21, 2016

@diogobeda there are some merge conflicts.

@diogobeda
Copy link
Author

Sorry, I've been very busy these days. Only gonna be able to look into it on the weekend.

@timothyis timothyis added the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Jul 29, 2016
@matheuss matheuss closed this Aug 29, 2016
@matheuss matheuss removed the 👩‍🔬 Status: In Progress Issue or PR is currently active and work is in progress label Aug 29, 2016
@ekmartin
Copy link
Contributor

ekmartin commented Sep 6, 2016

Reducer tests would be immensely useful, and would be really good for confirming the behaviour of stuff like #693. @diogobeda: if you don't think you'll have time during the next weeks I can rebase and finish this up for you, let me know. Looks like a really clean implementation (although I agree that the describe/it nesting gets a bit heavy at points).

@diogobeda
Copy link
Author

Oh, the irony! I didn't have time to finish this cause I'm working on a talk I'm giving this month about testing redux. Sorry to leave it like this, guys.
@ekmartin I don't think I can dedicate a good amount of time to this, indeed. You can take it from here, if you want. Is there something I could do to make it smoother for you?

@matheuss
Copy link
Member

matheuss commented Sep 6, 2016

Don't worry @diogobeda!! Whenever you, @ekmartin or someone else have time to work on this, a reopen/new PR is more than welcome 😄

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

6 participants