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

[GSoC'21] Pr:6 . JSON saver tests #94

Merged
merged 15 commits into from Aug 6, 2021
Merged

Conversation

specter25
Copy link
Collaborator

GSoC'2021 Pull Request 5 :
Deliverables Completed :

  • complete unit tests on Jsonsaver
  • fix issues raised in Gsoc pr 5 related to Jsonsaver

- Unnecessary functioanlity of maintinging the order of properyies removed
- usage of map[string]interafce{} introduced

Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
- Unit Tests fro creation info function

Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
- Write units tests for render functions for snippets ,packages , files , reviews , relationships ,annotations

Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
- Checksums algorithm had to be parsed to string from spdx.ChecksumAlgorithm

Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
- added test to renderfile function
- added tests to renderrelationships function

Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
- Check whether an entire spdx document is saved correctly or not

Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
@specter25
Copy link
Collaborator Author

cc @swinslow I have made all the changes that you suggested in the previous meeting and moreover complete the unit tests as well as the complete functionality tests

…tions

Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
Copy link
Member

@swinslow swinslow left a comment

Choose a reason for hiding this comment

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

Hi @specter25, great work on this PR as always. I've got a few minor comments on particular bits which should show up in this review.

Unfortunately, there's one larger issue with the tests that I'm seeing which is pretty significant, and is randomly causing the tests to fail from time to time when I re-run them (even though they didn't fail in the CI when this PR was submitted). I'll explain below what the issue is. Because it's causing tests to fail, I'd suggest addressing it before I merge this PR into the json branch. Take a look at my comments below and let me know if you have questions!

= = =

The problem I'm seeing is that, when I re-run the tests repeatedly, some of them are failing from time to time (though sometimes they are passing). You can re-run the tests on your local machine by running go clean -testcache to clear the testing cache, followed by go test ./... as usual.

The problems I'm seeing appear to be where the items in a slice are getting saved into different orders on each test run. The test code expects them to appear in a specific order that you define in the test; and if they are in that order, then it passes, but sometimes they are in a different order.

Specifically, it's where the items are stored in a map in the tools-golang SPDX data model, but in an array (or slice) in the SPDX JSON format. Because of the difference in data structures, you are (correctly) doing a for ... range over the map, and then appending the results into a slice to get rendered in JSON as an array.

The problem is that when doing for ... range over a map, Golang returns the map contents in a random order. You can see this article for more details, or this shorter one (under 'Iteration order') as well.

So because the for ... range call returns the map contents in a random order, they will be randomly ordered into the slice as well. And that means that when you eventually do the DeepEqual call to check the results, they will sometimes pass and sometimes fail, depending on the random result.

Here's a couple of specific areas where I'm seeing this error:

To be clear, there could be others; those are just the ones I'm seeing so far. Basically, anywhere in the saver code where you are doing a for ... range over a map and storing the results into a slice, I'm guessing this could occur.

I think there's a couple of ways you could go about fixing this.

One option would be in the saver code: every time you create a slice from a map, always make sure the slice is sorted in the same order. This could be done for example by doing the for ... range over the map just to get the keys; then sorting the keys; and then iterating over the keys to get the values. The blog post I mentioned above explains how to do this, under the "Iteration order" section. If you do this, then the output from the render functions should always be consistent, and then the DeepEqual calls can always check for that consistent output.

A second option could be to have the DeepEqual call somehow ignore the ordering of slices when it compares the result to the expected structure. I'm not sure whether there's a straightforward way to do that; and it might not be a good idea anyway, in case there are any instances where ordering in slices actually does matter. (I don't know of any for SPDX at the moment.)

So I'd lean towards the first option, though if you have another thought, I'm happy to hear it!

tests := []struct {
name string
args args
wantW string
Copy link
Member

Choose a reason for hiding this comment

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

Is wantW being used here? I see that wantErr is being assigned and checked below, but it looks like this isn't actually checking to see whether the output of Save2_2 matches what's expected.

That might be intentional, since we can't be certain about the exact output of bytes from the JSON marshaller. And I believe you're checking the actual contents of the saver output in the other tests in the subdirectory. If that's the case, though, then perhaps it makes sense to remove wantW here to avoid confusion?

wantW string
wantErr bool
}{
// TODO: Add test cases.
Copy link
Member

Choose a reason for hiding this comment

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

Here and for the other test files below -- are you still planning to add more test cases? If so, fine to leave this in, but if not then once you're done I'd suggest removing these comments.

}
for k, v := range tt.want {
if !reflect.DeepEqual(tt.args.jsondocument[k], v) {
t.Errorf("Load2_2() = %v, want %v", tt.args.jsondocument[k], v)
Copy link
Member

Choose a reason for hiding this comment

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

Should Load2_2() be renderCreationInfo2_2() in this comment instead?

…xamples

- bug fixes in json saver done as well
- test covereage of jsonparser increased

Signed-off-by: Ujjwal Agarwal <ujjwalcoding012@gmail.com>
@swinslow
Copy link
Member

swinslow commented Aug 6, 2021

Hi @specter25, from our separate chat I understand that you're planning to submit a subsequent PR to handle the testing issue I mentioned above. So I'm going ahead and merging this one into the json branch. Thanks!

@swinslow swinslow merged commit 25bc95b into spdx:json Aug 6, 2021
@swinslow swinslow added this to the 0.3.0 milestone Aug 16, 2021
@swinslow swinslow added the enhancement New feature or request label Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants