-
Notifications
You must be signed in to change notification settings - Fork 54
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
Conversation
- 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>
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>
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.
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 append
ing 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:
- when rendering all Files into the "files" array:
for k, v := range doc.UnpackagedFiles { - when rendering a file's checksums:
for _, value := range v.FileChecksums {
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!
jsonsaver/jsonsaver_test.go
Outdated
tests := []struct { | ||
name string | ||
args args | ||
wantW string |
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.
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?
jsonsaver/jsonsaver_test.go
Outdated
wantW string | ||
wantErr bool | ||
}{ | ||
// TODO: Add test cases. |
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.
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) |
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.
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>
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! |
GSoC'2021 Pull Request 5 :
Deliverables Completed :