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

Refactor SwitchRouter tests #34

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@slorber
Copy link
Member

commented Feb 14, 2019

Follow up of the former backbehavior refactor proposal to make tests more maintainable and readable

return { applyAction, navigateTo, back, getState, getSubState };
};

const getSubStateResurcive = (state, level = 1) => {

This comment has been minimized.

Copy link
@slorber

slorber Feb 14, 2019

Author Member

Added this helper because there was a recuring pattern to access deeply nested date

});

test('handles initialRoute backBehavior', () => {
const router = getExampleRouter({ backBehavior: 'initialRoute' });
const { navigateTo, back, getState } = getRouterTestHelper(
getExampleRouter({ backBehavior: 'initialRoute', initialRouteName: 'B' })

This comment has been minimized.

Copy link
@slorber

slorber Feb 14, 2019

Author Member

added initialRouteName param in router here, because the test does not ensure this param is actually wired to anything and could only fallback to first route instead of using the param

@@ -13,6 +13,7 @@
"scripts": {
"pretest": "yarn lint && yarn build",
"test": "jest",
"test:dev": "jest --watch",

This comment has been minimized.

Copy link
@slorber

slorber Feb 14, 2019

Author Member

simplify testing for developers, because running pretest can be annoying and will make the tests not start if I have a lint error like an unused import or whatever

This comment has been minimized.

Copy link
@satya164

satya164 Apr 8, 2019

Member

why not run yarn test --watch? :)

This comment has been minimized.

Copy link
@slorber

slorber Apr 8, 2019

Author Member

@satya164 because I don't want to run lint/pretest, just the tests. Dev feedback I wanted was faster this way

This comment has been minimized.

Copy link
@satya164

satya164 Apr 8, 2019

Member

@slorber yea I was annoyed by that pretest too. maybe worth moving that to yarn ci or something

let state = router.getStateForAction(initAction);

const applyAction = action => {
debug && console.debug('\n');

This comment has been minimized.

Copy link
@slorber

slorber Feb 14, 2019

Author Member

Can remove these debug logs if needed. Just find them convenient when I'm not sure what's happening. Probably need this because I'm not really familiar with a proper test debugging workflow.

This comment has been minimized.

Copy link
@slorber

slorber Feb 14, 2019

Author Member

Actually just found this and it seems works fine in my IDE :D https://jestjs.io/docs/en/troubleshooting

This comment has been minimized.

Copy link
@satya164

satya164 Apr 8, 2019

Member

Let's remove these logs

@brentvatne brentvatne self-requested a review Feb 19, 2019

@slorber

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

Hi @brentvatne @ericvicenti did you have time to check this PR?

@satya164

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

I'll try to review it today.

@satya164 satya164 self-requested a review Mar 22, 2019

let state = router.getStateForAction(initAction);

const applyAction = action => {
debug && console.debug('\n');

This comment has been minimized.

Copy link
@satya164

satya164 Apr 8, 2019

Member

Let's remove these logs

Show resolved Hide resolved src/routers/__tests__/routerTestHelper.js Outdated
Show resolved Hide resolved src/routers/__tests__/routerTestHelper.js Outdated
Show resolved Hide resolved src/routers/__tests__/routerTestHelper.js Outdated

satya164 and others added some commits Apr 8, 2019

Update src/routers/__tests__/routerTestHelper.js
Co-Authored-By: slorber <slorber@users.noreply.github.com>
Update src/routers/__tests__/routerTestHelper.js
Co-Authored-By: slorber <slorber@users.noreply.github.com>
Update src/routers/__tests__/routerTestHelper.js
Co-Authored-By: slorber <slorber@users.noreply.github.com>
@slorber

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Thanks for the review @satya164

Forgot to remove the logs :'(

I'm on holiday for 2 weeks with only a phone so it's a bit hard to do changes :) if the PR is fine and you can remove the logs for me that would be cool

slorber added some commits May 6, 2019

Merge branch 'master' into refactor-tests
# Conflicts:
#	package.json
#	src/routers/__tests__/SwitchRouter-test.js
@slorber

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Hi @satya164

I did the cleanup and fixed the conflits with jumpTo

Also noticed you used jumpTo in some of the tests but I'm not totally sure to understand why some of them use jumpTo while some others still use navigateTo. During the merge I let navigateTo everywhere but if you explain which one should I use when I can change

@satya164

This comment has been minimized.

Copy link
Member

commented May 8, 2019

@slorber Thanks. I'll try to check this tomorrow.

I'm not totally sure to understand why some of them use jumpTo while some others still use navigateTo

It's random really

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.