-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Min max npm #18796
Min max npm #18796
Conversation
KarishmaGhiya
commented
Nov 23, 2021
•
edited
Loading
edited
- Update logic to update azure package dependencies from outside the repo too
- Update logic to run min-max testing on third party dependencies
- Update integration-test - node script to take the timeout from the package's package.json - Fixes [min-max] - Respect the package's timeout when running integration test #16731
- Verify logic working or not on identity and communication-chat package
- Fixes [Dependency-Testing] Enable minmax testing on packages from NPM feed. #14950
- Verification run - https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1258044&view=results (If you see one failing test, it's because that one test is failing on ubuntu - unrelated to min-max)
} | ||
} | ||
else if (versionType === "same") { | ||
return packageJsonDepVersion; | ||
} |
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.
Good improvements!
} | ||
testPackageJson.dependencies = depList; | ||
|
||
for (const package of Object.keys(packageJsonContents.devDependencies)) { | ||
testPackageJson.devDependencies[package] = packageJsonContents.devDependencies[package]; | ||
if (package.startsWith("@azure/")) { | ||
if (package.startsWith("@azure/") || package.startsWith("@azure-rest/")) { |
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.
Could we test the package sdk-type
field instead of looking at the namespace prefix?
dev-tool uses an @azure/
namespace, but it isn't a client
SDK.
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.
For that I will have to read the package.json contents of the package
- might increase the parsing time. I think it should be ok. Since I am anyways filtering out the utility packages - so not running min-max on the dev-tool and other utility packages.
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.
Alright, as long as we've got a list then it doesn't matter very much.
@@ -9,7 +9,7 @@ | |||
"scripts": { | |||
"build": "tsc -p .", | |||
"integration-test:browser": "karma start --single-run", | |||
"integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter mocha-multi-reporter.js --timeout 350000 --full-trace 'dist/**/*.js' --exit", | |||
"integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter mocha-multi-reporter.js --timeout 350000 --full-trace \"dist-esm/**/*.spec.js\" --exit", |
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.
Wouldn't this also run browser tests?
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.
dist-esm/**/{,!(browser)/**/}*.spec.js
- How about this?
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.
Thanks for addressing my comments.
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.
Looks good! I love the comments ❤️ Thank you!
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.
🚀
dev-Sentinel-2022-05-01-preview (Azure#18796) * Adds base for updating Microsoft.SecurityInsights from version preview/2022-04-01-preview to version 2022-05-01-preview * Updates readme * Updates API version in new specs and examples * Added fileImports.json (Azure#18540) * Added fileImports.json * add example files * move example files * delete old example files * fix get file import by id example * fix delete file import example * fix typos * resolve issues * fix date * more issue resolving * . * . * add location to delete example * support 200 * resolve comments * update example * remove whitespace * readonly * operationId * revert file imports (Azure#18663) * add securityMLAnalyticsSettings to securityInsights (Azure#18679) * add securityMLAnalyticsSettings to securityInsights * update for operationIdNounConflictingModelNames * update plurality * update * add settingsdefinintionid field for anomalySettings * Add Anomaly to timeline items (Azure#18675) * Add Anomaly to timeline items * Add exmaple of anomaly * Add x-ms-identifiers * fix issue fix x-m-id issue. fix lint * Change enum name * REmove timeline from inceidents * swap enum definistion and name * Align SKU descriptions (Azure#18874) * Align SKU descriptions * Remove Sku model from Settings as it's not in use and has conflict with another trype. * Update AutomationRules.json (Azure#18630) * Update automation rules swagger * Update examples * fix examples * fix naming typo * run prettier * run prettier on missing file * remove logic app arm id from required * remove a tag with errors (Azure#18916) Co-authored-by: moranraz <[email protected]> Co-authored-by: jungph808 <[email protected]> Co-authored-by: rpressburger <[email protected]> Co-authored-by: ityankel <[email protected]> Co-authored-by: Dapeng Zhang <[email protected]>