-
Notifications
You must be signed in to change notification settings - Fork 475
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
Commit defs #758
base: main
Are you sure you want to change the base?
Commit defs #758
Conversation
|
||
all: lib/defs.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced the default "all" target with an "error" target because we don't want people running make and unintentionally generating new lib/defs.js
coverage: $(NYC) lib/defs.js | ||
$(NYC) --reporter=lcov --reporter=text $(_MOCHA) -u tdd -R progress test/ | ||
coverage: $(NYC) | ||
$(NYC) --clean --reporter=lcov --reporter=text $(_MOCHA) -u tdd --exit -R progress test/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the clean target deleting the .coverage directory, I thought it better to have nyc do it.
Mocha was hanging so I added the --exit argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is causing Mocha to hang? It's generally a good idea to avoid --exit
, because otherwise we can miss problems with resource disposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can work it out. Most likely a connection to the broker not being closed in one of the tests.
@echo "HTML report at file://$$(pwd)/coverage/lcov-report/index.html" | ||
|
||
lib/defs.js: clean bin/generate-defs test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to be sure the new lib/defs.js were tested post generation
There are some good arguments for committing the generated lib/defs.js file
I've also decided to include the amqp-rabbit-0.9.1.json specification. It's not necessary, but only a small increase to the overall package size and might prove useful for someone trying to understand why an operation is missing