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

Nice GQL tests config #854

Merged
merged 10 commits into from Aug 13, 2018
Merged

Conversation

tpscrpt
Copy link
Contributor

@tpscrpt tpscrpt commented Aug 13, 2018

Proposed changes

Goal was to optimize the tests to remove unrelated plugins, the required ones are Logger, Logger-Winston and I've found it easier to set a local variable with the require('../lib').register(container, options) as the graphql plugin rather than include it into the config/plugins.js copied from core/config/testnet/* (I was getting undefined issues cause I suck at async) -- now tests pass in a few seconds.

There's an issue with tests running indefinitely if you don't use forceExit which I think the core-container causes (can't see why most plugins would return the same error without --detectOpenHandles and no --forceExit) so I've changed the package.json file to use both and --runInBand (don't know what --rIB does, saw it in some other plugins, thought it was "good").

Tried to remove as much redundancy and will be working on this further.

Types of changes

  • [ x ] Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [ x ] Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • [ x ] Test (adding missing tests or fixing existing tests)
  • Other... Please describe:

Checklist

  • [ x ] I have read the CONTRIBUTING documentation
  • [ x ] Lint and unit tests pass locally with my changes (somereason I'm getting trolled)
  • [ x ] I have added tests that prove my fix is effective or that my feature works
  • [ x ] I have added necessary documentation (if appropriate)

In line with #816

@@ -9,7 +9,7 @@
"main": "lib/index.js",
"scripts": {
"build:docs": "../../node_modules/.bin/jsdoc -c jsdoc.json",
"test": "cross-env ARK_ENV=test jest --runInBand --detectOpenHandles",
"test": "cross-env ARK_ENV=test jest --forceExit --runInBand --detectOpenHandles",
Copy link
Contributor

Choose a reason for hiding this comment

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

detectOpenHandles replaces forceExit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@tpscrpt
Copy link
Contributor Author

tpscrpt commented Aug 13, 2018

Ok, sorry I'll read some jest, I'll remove detectOpenHandles too

@faustbrian
Copy link
Contributor

Also please use [x] instead of [ x ] in your PRs or the to-do lists won't work. You can see that it doesn't render checkboxes.

@@ -9,7 +9,7 @@
"main": "lib/index.js",
"scripts": {
"build:docs": "../../node_modules/.bin/jsdoc -c jsdoc.json",
"test": "cross-env ARK_ENV=test jest --forceExit --runInBand --detectOpenHandles",
"test": "cross-env ARK_ENV=test jest --runInBand --forceExit",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still wrong, forceExit is replaced by detectOpenHandles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said, use --detectOpenHandles instead of --forceExit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

works locally now with proper plugin management (doesnt hang with detectopenhandles)

@codecov-io
Copy link

Codecov Report

Merging #854 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #854      +/-   ##
==========================================
- Coverage   43.96%   43.94%   -0.02%     
==========================================
  Files         426      426              
  Lines        7033     7033              
  Branches      894      894              
==========================================
- Hits         3092     3091       -1     
- Misses       3398     3399       +1     
  Partials      543      543
Impacted Files Coverage Δ
packages/core-p2p/lib/utils/check-ntp.js 78.57% <0%> (-7.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8102f23...913159c. Read the comment docs.

@faustbrian faustbrian merged commit e6f8d73 into ArkEcosystem:master Aug 13, 2018
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

3 participants