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

Chores/test-coverage-setup-#151817717 #186

Merged
merged 9 commits into from
Mar 1, 2019

Conversation

chrisdolendo
Copy link
Contributor

No description provided.

@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 21, 2019 20:17 Inactive
@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 21, 2019 20:30 Inactive
@chrisdolendo chrisdolendo force-pushed the chores/test-coverage-setup-#151817717 branch from 56c1977 to 81d17c1 Compare February 21, 2019 20:33
@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 21, 2019 20:33 Inactive
@chrisdolendo chrisdolendo force-pushed the chores/test-coverage-setup-#151817717 branch from 81d17c1 to 519f701 Compare February 21, 2019 20:54
@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 21, 2019 20:54 Inactive
@akegan
Copy link
Contributor

akegan commented Feb 22, 2019

can we setup codeclimate independently of semaphore? as a separate github integration? It might make it easier to understand the results

Here's some documentation: https://docs.codeclimate.com/docs/github-pull-requests#section-pull-request-statuses

@akegan
Copy link
Contributor

akegan commented Feb 22, 2019

Looks like code climate costs money for >4 users committing code https://codeclimate.com/quality/pricing/. I wonder how that was handled in the past

We could just do chris, cat, me, and martin, and not analyze josh's code? See how it goes and if we find it super valuable we can potentially procure it.

alternatively, are there free test coverage reporting packages for ruby and js?

@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 22, 2019 23:02 Inactive
@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 25, 2019 14:06 Inactive
@exygy-dev exygy-dev had a problem deploying to dahlia-lap-full-pr-186 February 25, 2019 15:08 Failure
@exygy-dev exygy-dev had a problem deploying to dahlia-lap-full-pr-186 February 25, 2019 15:10 Failure
@chrisdolendo
Copy link
Contributor Author

Great we have coverage now after configuring semaphore + installing simplecov (ruby) and istanbul (for lcov js reporting) -- but for some reason some JS tests fail even if no actual feature code was changed. 🤔

@akegan
Copy link
Contributor

akegan commented Feb 25, 2019

@chrisdolendo we had been getting some red text running tests locally. seemed like warning-type things. I wonder if changing the packages somehow affected the tolerance of those tests

@chrisdolendo
Copy link
Contributor Author

@akegan good call I will look into the packages

@chrisdolendo chrisdolendo force-pushed the chores/test-coverage-setup-#151817717 branch from fa03277 to 30c775a Compare February 26, 2019 02:25
@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 26, 2019 02:25 Inactive
@chrisdolendo chrisdolendo force-pushed the chores/test-coverage-setup-#151817717 branch from 30c775a to 9569003 Compare February 26, 2019 05:10
@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 26, 2019 05:10 Inactive
@chrisdolendo
Copy link
Contributor Author

chrisdolendo commented Feb 26, 2019

I figured out the issue with the e2e test from research -- essentially using puppeteer (which e2e tests uses) and jest with code coverage enabled breaks the test.

You can't activate coverage for E2E. It is very complicated to do because you don't have direct access to the tested code.

I tried implementing puppeteer-to-istanbul but it didn't work, because istanbul is already baked in to jest so it conflicted, and also needs the latest node version.

SO, I turned on coverage just for the unit tests and did not include e2e in it until there a solution for jest.

Code coverage went up to 76% after including JS tests.

@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 26, 2019 05:26 Inactive
@akegan akegan temporarily deployed to dahlia-lap-full-pr-186 February 27, 2019 21:05 Inactive
@@ -65,10 +65,10 @@
"webpack-dev-server": "^2.9.3"
},
"scripts": {
"unit": "jest --testPathPattern=.*.test.js$",
"unit": "jest --testPathPattern=.*.test.js$ --coverage",
Copy link
Contributor

Choose a reason for hiding this comment

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

with python tests we generally ran the tests and the coverage separately. Is there a reason why you're combining it into one line here?

Copy link
Contributor Author

@chrisdolendo chrisdolendo Feb 27, 2019

Choose a reason for hiding this comment

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

@akegan to feed two birds with one seed - but I can separate them -- would you like me to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern is that it might be hard to distinguish on semaphore if the tests are failing due to tests failing or due to coverage not passing. would that be an issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just reports the coverage that gets passed to code climate, it doesn't keep it from passing or failing. Code climate will let us know in the PR if it doesn't pass. e.g. this PR shows that the new changes don't pass the Diff Test Coverage:
screen shot 2019-02-27 at 1 35 20 pm

@exygy-dev exygy-dev temporarily deployed to dahlia-lap-full-pr-186 February 27, 2019 21:42 Inactive
@chrisdolendo
Copy link
Contributor Author

I DID IT!!! Had to update a sempahore command. We now are up 76% test coverage.
screen shot 2019-02-28 at 8 34 53 am

Copy link
Contributor

@akegan akegan left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @chrisdolendo !

@chrisdolendo chrisdolendo merged commit d4ec2e8 into master Mar 1, 2019
@akegan akegan deleted the chores/test-coverage-setup-#151817717 branch April 5, 2019 21:55
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.

3 participants