-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add new functional tests suite #769
Conversation
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.
Minor.
The structure looks good to me.
test/functional/config-test.js
Outdated
deviceRegistrationDuration: 'P1M', | ||
defaultType: 'Thing', | ||
defaultResource: '/iot/json', | ||
compressTimestamp: true |
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.
why it appears again? in any case should be false.
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.
It is inherited from previous unit tests config file. I will remove here and open a new PR to remove it from the rest of config files
test/functional/functional-tests.js
Outdated
}); | ||
}); | ||
|
||
describe('Basic group provision with attributes', function () { |
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 propose to refer to the attributes as "active attributes"
After committing ce2e261 tests are more verbose, you can check how an expectation that differs from the CB payload looks like: It is made to intentionally make tests fail for demonstration purposes. CC: @manucarrace @mrutid |
package.json
Outdated
"test:coverage": "nyc --reporter=lcov mocha -- --recursive 'test/**/*.js' --reporter spec --exit", | ||
"test:coveralls": "npm run test:coverage && cat ./coverage/lcov.info | coveralls && rm -rf ./coverage", | ||
"test:watch": "npm run test -- -w ./lib", | ||
"watch": "watch 'npm test && npm run lint' ./lib ./test" | ||
}, | ||
"devDependencies": { | ||
"async-mqtt": "~2.6.3", | ||
"chai": "^4.3.10", |
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.
Don't use ^. Always ~
;)
@@ -29,12 +29,15 @@ | |||
"prettier:text": "prettier 'README.md' 'docs/*.md' 'docs/**/*.md' --no-config --tab-width 4 --print-width 120 --write --prose-wrap always", | |||
"start": "node ./bin/iotagent-json", | |||
"test": "nyc --reporter=text mocha --recursive 'test/**/*.js' --reporter spec --timeout 5000 --ui bdd --exit --color true", | |||
"test:functional": "nyc --reporter=text mocha --recursive 'test/functional/*.js' --reporter spec --timeout 5000 --ui bdd --exit --color true", |
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.
A GitAction for this test should be included.
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.
No real needed since they are executed as part of normal tests 'test/**/*.js'
. In other words, functional tests are executed as part of normal tests (npm test
).
Unless we want to add a new section under CI tests, it is not required
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.
In that case, why don't just keep test
target and remove test:functional
?
Not sure if having overlapping targets is a good idea...
The test that expects the measures includes metadata with TimeInstant when
|
As discussed at Teams, a test/functional/README.md would be a good idea. |
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.
LGTM
This PR includes a new "functional" test suite. It should:
Things to have into account in this suite: