-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix: Capture the npm_package_version #3198
Conversation
Signed-off-by: ebadiere <[email protected]>
Test Results 20 files - 3 259 suites - 34 33m 12s ⏱️ - 4m 24s For more details on these failures, see this check. Results for commit bbce9f6. ± Comparison against base commit dfcb7cf. This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
file to allow test framework to load the value in the config service. Also some clean up. Signed-off-by: ebadiere <[email protected]>
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
@ebadiere could you run |
Signed-off-by: ebadiere <[email protected]>
Quality Gate passedIssues Measures |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3198 +/- ##
==========================================
+ Coverage 83.10% 83.39% +0.29%
==========================================
Files 69 66 -3
Lines 4444 4283 -161
Branches 873 835 -38
==========================================
- Hits 3693 3572 -121
+ Misses 487 471 -16
+ Partials 264 240 -24
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* fix: Capture the npm_package_version Signed-off-by: ebadiere <[email protected]> * fix: Adding npm_package_version at the root package.json file to allow test framework to load the value in the config service. Also some clean up. Signed-off-by: ebadiere <[email protected]> * chore: Updated package-lock.json Signed-off-by: ebadiere <[email protected]> --------- Signed-off-by: ebadiere <[email protected]>
* fix: Capture the npm_package_version (#3198) * fix: Capture the npm_package_version Signed-off-by: ebadiere <[email protected]> * fix: Adding npm_package_version at the root package.json file to allow test framework to load the value in the config service. Also some clean up. Signed-off-by: ebadiere <[email protected]> * chore: Updated package-lock.json Signed-off-by: ebadiere <[email protected]> --------- Signed-off-by: ebadiere <[email protected]> * fix: Added the correct version to the root level package.json and rebuilt the package-lock.json. Signed-off-by: ebadiere <[email protected]> --------- Signed-off-by: ebadiere <[email protected]>
Description:
The ConfigService was using an all caps version of the
npm_package_version
of the env var definition but in a node context the env var is lowercase:npm_package_version
. Nothing was being returned from theweb3_clientVersion
method.Related issue(s):
Fixes #3197
Notes for reviewer:
Simply changing the case of the env var definition in the
GlobalConfig.ts
class prevents updating conditional logic that already works. Adding the version number to the root package.json file allows it to be loaded by theConfigService.ts
which loads before the acceptance tests framework.Checklist