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

test(nodejs): Add tests for checking import graphs #337

Merged
merged 7 commits into from
Oct 10, 2018
Merged

Conversation

zlav
Copy link
Member

@zlav zlav commented Oct 9, 2018

Created from issue [#329]

I added a folder called normal, welcom to hear name changes but this is normal operation for nodejs analyzation. This holds the node modules and package for the analyzer to grab information from.

I also privitized a previous function called AssertImport which was only used in this package and moved it to the end of the file. Github didn't notice this and that is the reason for the large delta at the end of the file.

@codecov
Copy link

codecov bot commented Oct 9, 2018

Codecov Report

Merging #337 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #337   +/-   ##
=======================================
  Coverage   41.67%   41.67%           
=======================================
  Files          58       58           
  Lines        2644     2644           
=======================================
  Hits         1102     1102           
  Misses       1433     1433           
  Partials      109      109

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61ce589...cf59093. Read the comment docs.

@zlav zlav requested a review from microsoftly October 9, 2018 23:54
@@ -13,6 +13,46 @@ import (
"github.com/fossas/fossa-cli/pkg"
)

// testNDepsTransitiveImports checks that Analyze() returns the correct transitive deps
// by asserting on transitive dependencies
func testNDepsTransitiveImports(t *testing.T) {

Choose a reason for hiding this comment

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

testNDepsTransitiveImports is unused

assertImport(t, chaiProject.Imports, "type-detect", "4.0.8")
}

func assertPackageReturnImports(t *testing.T, packages map[pkg.ID]pkg.Package, name, revision string) pkg.Package {

Choose a reason for hiding this comment

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

func assertPackageReturnImports is unused

// graph.Deps object by asserting on direct and transitive dependencies
func TestNodeAnalyze(t *testing.T) {
m := module.Module{
Dir: filepath.Join("testdata", "normal"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is incorrect. A BuildTarget should be provided. This passes because NPM.List is mocked. You could potentially use a single mock and point to a different mock output based on the BuildTarget provided

@@ -13,6 +13,50 @@ import (
"github.com/fossas/fossa-cli/pkg"
)

// TestNodeAnalyze checks that Analyze() runs correctly and returns the correct
// graph.Deps object by asserting on direct and transitive dependencies
func TestNodeAnalyze(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be helpful to add a comment with the actual output of npm ls so it's human readable

analyzers/nodejs/nodejs_test.go Outdated Show resolved Hide resolved
analyzers/nodejs/nodejs_test.go Outdated Show resolved Hide resolved
analyzers/nodejs/nodejs_test.go Outdated Show resolved Hide resolved
analyzers/nodejs/testdata/normal/npm-ls-json.json Outdated Show resolved Hide resolved
analyzers/nodejs/testdata/transitive-deps/npm-ls-json.json Outdated Show resolved Hide resolved
@elldritch elldritch changed the title test(nodejs graph test) assert that nodejs constructs an accurate dependency graph test(nodejs): Add tests for checking import graphs Oct 10, 2018
Copy link
Contributor

@microsoftly microsoftly left a comment

Choose a reason for hiding this comment

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

A few nits left to change. I know we touched on this last night, but can you (again) verify that the structure of npm ls --json is the same for early (0.10.X), mid range (4.x.x), and recent (8.x.x) nodejs/npm versions?

@@ -13,6 +13,76 @@ import (
"github.com/fossas/fossa-cli/pkg"
)

/*
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Update this to be in a human readable form
e.g.

/*
    ├─┬ chai@4.1.2
    │ ├── assertion-error@1.1.0
    │ ├── check-error@1.0.2
    │ ├─┬ deep-eql@3.0.1
    │ │ └── type-detect@4.0.8
    │ ├── get-func-name@2.0.0
    │ ├── pathval@1.1.0
    │ └── type-detect@4.0.8
    └── type-detect@3.0.0
*/

analyzers/nodejs/nodejs_test.go Outdated Show resolved Hide resolved
analyzers/nodejs/testdata/transitive-deps/package.json Outdated Show resolved Hide resolved
@zlav zlav self-assigned this Oct 10, 2018
@zlav
Copy link
Member Author

zlav commented Oct 10, 2018

@microsoftly the format for npm ls --json has not changed throughout past versions of node and npm. However, an interesting note to point out is that npm has gotten increasingly better at recognizing transitive dependencies with newer releases.

Copy link
Contributor

@elldritch elldritch left a comment

Choose a reason for hiding this comment

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

Looks great. Minor changes, then ready to merge.

Can you confirm that this test fails for the current analysis? I want to make sure that this is a sufficiently complex fixture to catch the bug we saw.

analyzers/nodejs/nodejs_test.go Outdated Show resolved Hide resolved
analyzers/nodejs/nodejs_test.go Outdated Show resolved Hide resolved
analyzers/nodejs/nodejs_test.go Outdated Show resolved Hide resolved
@zlav
Copy link
Member Author

zlav commented Oct 10, 2018

@liftM I confirmed the tests fail for the previous analysis that contained the bug we fixed. Merging this commit now.

@zlav zlav merged commit a3792d0 into master Oct 10, 2018
@zlav zlav deleted the test/node-graph branch October 10, 2018 19:50
meghfossa added a commit that referenced this pull request Nov 12, 2021
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.

4 participants