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

Adding testing infrastructure #55

Merged
merged 1 commit into from Nov 7, 2019
Merged

Conversation

@ImmutableBox
Copy link
Collaborator

ImmutableBox commented Nov 6, 2019

This pull request sets up the testing infrastructure using Jest .

To run the test : npm test
I only created one simple test as a starting point. The test does check if the Redis server is open.

NOTE: npm test will most likely not end correctly with error:
"Jest did not exit one second after the test run has completed. This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with --detectOpenHandles to troubleshoot this issue"

(CTRL + C to exit test.)

This is because the Redis server is still online, closing the server after testing would fix the issue.
If there is a way to handle opening and closing Redis inside the project, this problem won't happen anymore <- CREATE NEW ISSUE TO SOLVE THIS

@cindyledev

This comment has been minimized.

Copy link
Collaborator

cindyledev commented Nov 6, 2019

I ran this test on my machine and it worked great. My first test failed because I was trying to open redis in a port that was already occupied. I killed the instance using the port and tried again and the second test was successful as expected.
Annotation 2019-11-06 153302

@cindyledev cindyledev self-requested a review Nov 6, 2019
@cindyledev

This comment has been minimized.

Copy link
Collaborator

cindyledev commented Nov 6, 2019

I just realized there's already a npm test script in the package.json file, be careful when you merge that you don't delete the test script that's already there

@ImmutableBox

This comment has been minimized.

Copy link
Collaborator Author

ImmutableBox commented Nov 6, 2019

I just realized there's already a npm test script in the package.json file, be careful when you merge that you don't delete the test script that's already there

Yup. Thanks, Ill rebase this pull request later today when I have time.

@ImmutableBox

This comment has been minimized.

Copy link
Collaborator Author

ImmutableBox commented Nov 7, 2019

@UltimaBGD I had to merge your ESLint pull request with my Jest testing infrastructure pull request. It would be great if you can look it over and give an approval. I changed npm test to call Jest instead of ESLint. ESLint can still be called with npm run eslint and npm run eslint-fix.

@ImmutableBox ImmutableBox requested a review from cindyledev Nov 7, 2019
@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 7, 2019

@ImmutableBox what do you mean by "I had to merge with your eslint pr"? You don't want to do that. If you need to test, that's fine locally, but you should rebase on it when it gets landed.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 7, 2019

To confirm: that PR has landed, so you can just rebase on master, don't merge.

@ImmutableBox

This comment has been minimized.

Copy link
Collaborator Author

ImmutableBox commented Nov 7, 2019

To confirm: that PR has landed, so you can just rebase on master, don't merge.

Yes, I rebase to master. I then used git merge master with the branch I was working on and fixed merging conflicts on package.json and package-lock.json. When I ran npm run eslint-fix it seemed to fix the formatting for all the other files.

This is what my git log looks like:
image

Where Adding testing infrastructure #42 is my first commit, Add ESLint with Airbnb style is what's currently on master, and the commit I just uploaded.

If this is wrong I can do it again.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 7, 2019

OK, so this is wrong. You've done a merge, and we don't want to merge while on a PR branch, only rebase. You want to update your commits so they are in line with the latest master, but not merge with it. The merge only happens when the PR gets approved and landed.

We can fix this together in class tomorrow if you want, or you can do it yourself. Basically, you want to rebase your 8b5731bf61ce781088a83ebcd522638527824d87 commit on master, and fix the conflicts to produce something similar to what your merge does. Then you need to git push origin issue42 -f.

@UltimaBGD

This comment has been minimized.

Copy link
Contributor

UltimaBGD commented Nov 7, 2019

You should be able to run Jest and ESlint in the same command with the "&&" operators, is there a reason why ESlint had to be taken out of the test?

@ImmutableBox

This comment has been minimized.

Copy link
Collaborator Author

ImmutableBox commented Nov 7, 2019

You should be able to run Jest and ESlint in the same command with the "&&" operators, is there a reason why ESlint had to be taken out of the test?

No, not really just following this blog he had it separated. Ill add it back in.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 7, 2019

@UltimaBGD the && operator won't work on all shells (e.g., many Windows shells). There are few work arounds for this:

  1. we can use pre and post scripts to run things before/after other steps, https://docs.npmjs.com/misc/scripts
  2. we could use https://www.npmjs.com/package/npm-run-all to allow for a set of steps

In general, we have to be careful that our scripts work on Windows, since so many common tricks like this will fail :(

@UltimaBGD

This comment has been minimized.

Copy link
Contributor

UltimaBGD commented Nov 7, 2019

Okay, I think a pre script would make sense for this. How do we want to go about adding that in? Get this merged as it is and then I make another PR with the prescript?

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 7, 2019

Given that nothing is depending on these tests yet, or even eslint for that matter, let's get this rebased (package* files need updating) and then do more PRs on top.

@UltimaBGD

This comment has been minimized.

Copy link
Contributor

UltimaBGD commented Nov 7, 2019

Alright, I will keep my eye on it and work on that after.

@ImmutableBox

This comment has been minimized.

Copy link
Collaborator Author

ImmutableBox commented Nov 7, 2019

Given that nothing is depending on these tests yet, or even eslint for that matter, let's get this rebased (package* files need updating) and then do more PRs on top.

I think I got it down.

git log:
image

I think I still have to make another commit or rebase, since Jest doesn't work anymore.

@humphd

This comment has been minimized.

Copy link
Contributor

humphd commented Nov 7, 2019

Yes, that looks right, good work. Just do whatever you have to in order to get Jest to work again, and then do git commit --amend --no-edit and update your current commit, then git push origin issue42 -f when you are ready to have us review.

@ImmutableBox

This comment has been minimized.

Copy link
Collaborator Author

ImmutableBox commented Nov 7, 2019

OK. Jest is now working with npm test, ill create a new issue for running both ESLint and Jest together with the comments provided.

Like before I ran npm run eslint and npm run eslint-fix so it seem to fix the spacing for the rest of the files.

Copy link
Contributor

humphd left a comment

One thing I notice.

@@ -5,7 +5,7 @@
"scripts": {
"eslint": "eslint --ignore-path .gitignore .",
"eslint-fix": "eslint --fix --ignore-path .gitignore .",
"test": "npm run eslint",
"test": "jest",

This comment has been minimized.

Copy link
@humphd

humphd Nov 7, 2019

Contributor

Leave the test script alone, and instead add one for jest:

"test": "npm run eslint",
"jest": "jest",

In the new bug you file we can combine these via a call to "pretest": "npm run eslint" and change test to run jest

This comment has been minimized.

Copy link
@ImmutableBox

ImmutableBox Nov 7, 2019

Author Collaborator

Fixed in c3d8a6a.

@humphd
humphd approved these changes Nov 7, 2019
Copy link
Contributor

humphd left a comment

The particular test we have here is not going to be one we want to keep long term, but I know that wasn't your goal. I say we take this and keep iterating on it in other PRs.

Great work, thanks for taking this on!

Copy link
Contributor

UltimaBGD left a comment

Looks good to me!

@ImmutableBox ImmutableBox merged commit 163f2d0 into Seneca-CDOT:master Nov 7, 2019
@ImmutableBox ImmutableBox deleted the ImmutableBox:issue42 branch Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Main
Done
5 participants
You can’t perform that action at this time.