feat: update tests and correct type definition #46

kyegupov · 2019-11-26T10:30:18Z

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

Corrects two mistakes:

some tests were using .sort() on a PkgInfo[] list. Without the comparison function, this sort was doing nothing. The tests were passing just by accident.
PkgInfo.version was set to null sometimes and was advertised as null-able in the README. This was fixed, now only undefined should be used.

Since it introduces a light breakage (correction) of the library interface, this is being submitted as "feat" rather than "fix" or "tests".

gjvis · 2019-11-26T12:19:13Z

@kyegupov why null? Is that valid/desirable? Where have you seen it used?

kyegupov · 2019-11-26T12:20:22Z

@gjvis

dep-graph/src/core/create-from-json.ts

Line 26 in 1f21cac

pkgs[id] = info.version ? info : { ...info, version: null } as any;

dep-graph/test/core/create-from-json.test.ts

Lines 762 to 765 in 34da452

    
             { name: 'foo', version: null }, 
        
           ]); 
        
           expect(depGraph.getDepPkgs().sort()).toEqual([ 
        
             { name: 'foo', version: null },

darscan · 2019-11-26T13:09:13Z

Just my 2 cents (disregard at will):

I'm not wild about the test helper abstraction. The fix could have been to simply add the already used depSort to the two lines that were missed. But either way, it's fine
This PR is really 2 commits: the test stuff, and the type change
The feature commit message could be better: think about how it will look in the release notes

kyegupov · 2019-11-26T13:24:56Z

@darscan the first thing I did was exactly adding missing "depSort". Turned out, it was way more than 2 lines: there also was a bunch of expect(pkgs.sort()).toEqual([..arrayLiteral..]) which also needed fixing. I've figured that having a clear helper method is a better strategy than hoping that the developer won't forget to add .sort(depSort) into every assertion.

Re two changes in one: I've discovered the "null" problem late, when writing the helper, and I'm not yet sure how to handle it: proceed with nulls or convert them to undefineds.

darscan · 2019-11-26T13:33:21Z

I've figured that having a clear helper method is a better strategy than hoping that the developer won't forget to add .sort(depSort) into every assertion

Cool, I'm sold

darscan · 2019-11-26T13:34:57Z

I'm not yet sure how to handle it: proceed with nulls or convert them to undefineds.

Yeh

> JSON.stringify({foo: null, bar: undefined})
'{"foo":null}'

kyegupov · 2019-11-26T13:35:59Z

yup, either change is slightly breaking something: types or implementation

darscan · 2019-11-26T13:42:05Z

yup, either change is slightly breaking something: types or implementation

yeh. Out of curiosity.. if you don't change anything.. does anything actually break?

kyegupov · 2019-11-26T13:52:28Z

@darscan the only things that could be broken are: pkg.version===null, pkg.hasOwnProperty('version'), JSON comparison, "deep equality" comparison in tests.

michael-go · 2019-11-26T20:03:25Z

src/legacy/index.ts

@@ -78,7 +78,7 @@ async function buildGraph(
    eventLoopSpinner: EventLoopSpinner,
    isRoot = false): Promise<string> {

-  const getNodeId = (name: string, version: string | undefined, hashId: string) =>
+  const getNodeId = (name: string, version: string|null|undefined, hashId: string) =>


should better have spaces around the | - that's how it appears in the docs: https://www.typescriptlang.org/docs/handbook/advanced-types.html#union-types.

michael-go · 2019-11-26T20:11:22Z

@kyegupov good catch that we actually set it to null in the code but the type is string | undefined.

note that this change breaks compilation in some of our current code-bases, but maybe it's ok? ...
maybe if we fix it, better fix ~~it to be just string | null to make it simpler~~ the code to set undefined instead of null if the version is nullish - and leave the type simple with .version?. Also @gjvis & @darscan think it feels more correct to have version "undefined" if it's unknown, rather than null ?

michael-go · 2019-11-27T15:07:30Z

src/core/create-from-json.ts

@@ -22,8 +22,7 @@ export function createFromJSON(depGraphData: DepGraphData): DepGraph {
  const pkgNodes: {[pkgId: string]: Set<string>} = {};

  for (const { id, info } of depGraphData.pkgs) {
-    // TODO: avoid this, instead just use `info` as is
-    pkgs[id] = info.version ? info : { ...info, version: null } as any;
+    pkgs[id] = info;


maybe it's better to actually set it to undefined if it's nullish to make sure it adhers the type?

Yep, why not.

michael-go · 2019-11-27T15:09:20Z

src/core/dep-graph.ts

@@ -161,6 +161,10 @@ class DepGraphImpl implements types.DepGraphInternal {
      otherDepGraph = createFromJSON(other.toJSON()) as types.DepGraphInternal;
    }

+    // In theory, for the graphs created by standard means, `_.isEquals(this._data, otherDepGraph._data)`
+    // should suffice, since node IDs will be generated in a predictable way.
+    // However, to support unconventional node IDs, we run our own deep


not only - it's also to:

compare across outputs of different (compatible) versions of the lib

be not sensitive to ordering as the data contains arrays

the array ordering should be fairly deterministic, I believe (but that hinges on the current V8 implementation)

michael-go

maybe we want to "coalesce" the null-ish versions we get when converting from a depTree to undefined? like e.g. in https://github.com/snyk/dep-graph/blob/master/src/legacy/index.ts#L48

kyegupov · 2019-12-04T12:16:59Z

@michael-go added coalecsion

snyksec · 2019-12-04T12:57:10Z

🎉 This PR is included in version 1.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

kyegupov requested a review from a team as a code owner November 26, 2019 10:30

kyegupov self-assigned this Nov 26, 2019

kyegupov force-pushed the feat/update-tests-and-types branch from 1a39daf to 4ea6430 Compare November 26, 2019 10:46

kyegupov requested review from darscan and michael-go November 26, 2019 12:14

michael-go reviewed Nov 26, 2019

View reviewed changes

feat: clarify Pkg.version type (undefined instead of null)

3c133ff

kyegupov force-pushed the feat/update-tests-and-types branch 4 times, most recently from c94ddaf to f56a991 Compare November 27, 2019 12:00

darscan approved these changes Nov 27, 2019

View reviewed changes

michael-go reviewed Nov 27, 2019

View reviewed changes

test: fix comparisons of PkgInfo arrays

2a7e541

kyegupov force-pushed the feat/update-tests-and-types branch from f56a991 to 2a7e541 Compare December 4, 2019 12:14

michael-go approved these changes Dec 4, 2019

View reviewed changes

kyegupov merged commit fc9ff9a into master Dec 4, 2019

kyegupov deleted the feat/update-tests-and-types branch December 4, 2019 12:52

snyksec added the released label Dec 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update tests and correct type definition #46

feat: update tests and correct type definition #46

kyegupov commented Nov 26, 2019 •

edited

gjvis commented Nov 26, 2019

kyegupov commented Nov 26, 2019

darscan commented Nov 26, 2019

kyegupov commented Nov 26, 2019 •

edited

darscan commented Nov 26, 2019

darscan commented Nov 26, 2019

kyegupov commented Nov 26, 2019

darscan commented Nov 26, 2019

kyegupov commented Nov 26, 2019

michael-go Nov 26, 2019

michael-go commented Nov 26, 2019 •

edited

michael-go Nov 27, 2019

kyegupov Dec 4, 2019

michael-go Nov 27, 2019

kyegupov Dec 4, 2019

michael-go left a comment

kyegupov commented Dec 4, 2019

snyksec commented Dec 4, 2019

feat: update tests and correct type definition #46

feat: update tests and correct type definition #46

Conversation

kyegupov commented Nov 26, 2019 • edited

What does this PR do?

gjvis commented Nov 26, 2019

kyegupov commented Nov 26, 2019

darscan commented Nov 26, 2019

kyegupov commented Nov 26, 2019 • edited

darscan commented Nov 26, 2019

darscan commented Nov 26, 2019

kyegupov commented Nov 26, 2019

darscan commented Nov 26, 2019

kyegupov commented Nov 26, 2019

michael-go Nov 26, 2019

Choose a reason for hiding this comment

michael-go commented Nov 26, 2019 • edited

michael-go Nov 27, 2019

Choose a reason for hiding this comment

kyegupov Dec 4, 2019

Choose a reason for hiding this comment

michael-go Nov 27, 2019

Choose a reason for hiding this comment

kyegupov Dec 4, 2019

Choose a reason for hiding this comment

michael-go left a comment

Choose a reason for hiding this comment

kyegupov commented Dec 4, 2019

snyksec commented Dec 4, 2019

kyegupov commented Nov 26, 2019 •

edited

kyegupov commented Nov 26, 2019 •

edited

michael-go commented Nov 26, 2019 •

edited