Skip to content
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

chore: Fix flags and add tests #36

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Conversation

jsumners-nr
Copy link
Contributor

No description provided.

@jsumners-nr jsumners-nr marked this pull request as ready for review September 17, 2024 14:05
)
import "nrversions"

func Test_RenderCompatDoc(t *testing.T) {
Copy link
Member

@bizob2828 bizob2828 Sep 17, 2024

Choose a reason for hiding this comment

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

this test is fine but i think we need a destructive test. I've found if a target lacks a dep in the tests stanza or a minSupported it fails to parse versions for a package.json

Copy link
Member

Choose a reason for hiding this comment

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

This is what I did to verify the destructive path

diff --git a/testdata/versioned/aws-sdk-v3/package.json b/testdata/versioned/aws-sdk-v3/package.json
index 18468eb..9cf10ca 100644
--- a/testdata/versioned/aws-sdk-v3/package.json
+++ b/testdata/versioned/aws-sdk-v3/package.json
@@ -9,7 +9,7 @@
     {"name": "@aws-sdk/client-dynamodb", "minAgentVersion": "8.7.1"},
     {"name": "@aws-sdk/lib-dynamodb", "minAgentVersion": "8.7.1"},
     {"name": "@aws-sdk/smithy-client", "minSupported": "3.47.0", "minAgentVersion": "8.7.1"},
-    {"name": "@smithy/smithy-client", "minSupported": "2.0.0", "minAgentVersion": "11.0.0"},
+    {"name": "@smithy/smithy-client", "minAgentVersion": "11.0.0"},
     {"name": "@aws-sdk/client-bedrock-runtime", "minAgentVersion": "11.13.0"}
   ],
   "version": "0.0.0",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is an original design decision. When we can't find a version, we assume the whole manifest is broken and bail on it:

if target.MinSupported == "" {
version, err := findMinimumSupported(target, pkg.Tests)
if err != nil {
return nil, err
}
lastVersion = version
} else {
version, err := semver.NewRange([]byte(target.MinSupported))
if err != nil {
return nil, fmt.Errorf("%s: could not parse minSupported string '%s'", pkg.Name, target.MinSupported)
}
lastVersion = &version
}
if lastVersion == nil {
return nil, fmt.Errorf("%s: %w", pkg.Name, ErrTargetMissing)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say that changing that behavior is outside of the scope of this PR. It is a definite change in the way the tool works.

Copy link
Member

Choose a reason for hiding this comment

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

that's fair

@jsumners-nr jsumners-nr merged commit f5dd326 into newrelic:main Sep 17, 2024
5 checks passed
@jsumners-nr jsumners-nr deleted the fixes branch September 17, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done: Issues recently completed
Development

Successfully merging this pull request may close these issues.

2 participants