Skip to content

Conversation

jason-fox
Copy link
Contributor

@jason-fox jason-fox commented Feb 7, 2020

Simple Grep rename of the descriptions of the NGSI v2 tests, to distinguish them from the NGSI-LD test suite

Also updated the NGSI-v1 test files which have equivalent tests to distinguish between v1, v2 and LD

Simple Grep rename of the descriptions of the NGSI v2 tests, to distinguish them from the NGSI-LD test suite
Simple Grep rename of the descriptions of the NGSI v2 tests, to distinguish them from the NGSI-LD test suite
@jason-fox
Copy link
Contributor Author

jason-fox commented Feb 11, 2020

I'm not sure of your PR philosophy here, given that this PR touches tests only should I:

a) Make minimal changes (as is)
b) Prettify the changed test files - to reduce the scope of #753
c) Update the tests to run using ES6 syntax - to reduce the scope of #831

};

describe('Expression-based transformations plugin', function() {
describe('NGSI-v2 - Expression-based transformations plugin', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure but many of these tests are "parallel" to existing ones in NGSIv1 (maybe @dcalvoalonso could confirm, as he did great part of the implementation).

In that case, maybe it's a good idea to add a 'NGSI-v1 - prefix also in those ones, for the sake of completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 718ca29

@fgalan
Copy link
Member

fgalan commented Feb 11, 2020

I'm not sure of your PR philosophy here, given that this PR touches tests only should I:

a) Make minimal changes (as is)
b) Prettify the changed test files - to reduce the scope of #753
c) Update the tests to run using ES6 syntax - to reduce the scope of #831

This case is different from #841 (comment) in the sense in this case the test files are not new but pre-existing.

In my opinion, the decision should depend on the impact on other people's ongoing works. In other words, are the test files you plan to change already being changed in some of the ongoing PRs we have in the repository (apart from yours and mine)?

@jason-fox
Copy link
Contributor Author

Current list of amended tests in outstanding PRs.

test/unit/ngsiv2/provisioning/device-provisioning-api_test.js 
test/unit/ngsiv2/provisioning/updateProvisionedDevices-test.js
test/unit/provisioning/device-group-api-test.js

test/unit/expressions/expressionCombinedTransformations-test.js
test/unit/expressions/jexlBasedTransformations-test.js
test/unit/expressions/jexlExpression-test.js
test/unit/ngsiv2/expressions/expressionCombinedTransformations-test.js
test/unit/ngsiv2/expressions/jexlBasedTransformations-test.js

test/unit/general/iotam-autoregistration-test.js
test/unit/provisioning/device-group-utils_test.js
test/unit/provisioning/singleConfigurationMode-test.js

Updated the NGSI-v1 test files which have equivalent NSGI-v2 test  to distinguish between v1, v2 and LD
@jason-fox jason-fox changed the title Prepend NGSI-v2 to the v2 tests Prepend NGSI-v1 to the v1 tests, NGSI-v2 to the v2 tests Feb 11, 2020
@jason-fox
Copy link
Contributor Author

are the test files you plan to change already being changed in some of the ongoing PRs

Since the update now touches both NGSI-v1 and NGSI-v2 the answer would be YES
I'd prefer just to complete the title changes for now.

Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan fgalan merged commit a22bb62 into telefonicaid:master Feb 11, 2020
jason-fox added a commit to jason-fox/iotagent-node-lib that referenced this pull request Feb 11, 2020
@jason-fox jason-fox deleted the feature/v2-test-name branch February 20, 2020 15:19
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