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.

Sign up for GitHub

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

Add integration tests with skip lists for failing tests #33

Merged
merged 12 commits into from
May 29, 2020
Merged

Add integration tests with skip lists for failing tests #33

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
10543fd
Add integration tests with skip lists for failing tests
May 20, 2020
b80c6f9
Increase code readability, change regex with HasSuffix
May 22, 2020
db465ed
Refactor round trip tests
May 22, 2020
d8e8816
Updating comments, messages, renaming methods and variables,
May 26, 2020
435dba9
Round trip test logic change
May 27, 2020
ae74409
Round trip test logic change option 2
May 27, 2020
534d221
Improve Round trip test - update skip list - refactor
May 27, 2020
40ed246
Update skip list
May 27, 2020
71c5d5a
Handle Type.null - add error handling - update skip lists
May 27, 2020
f9bff48
Update comments
May 28, 2020
b1e2c09
Update comments - change signature of a helper function
May 29, 2020
70845d0
Merge branch 'master' into integration-test
zslayton May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 75 additions & 57 deletions ion/integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,47 +40,51 @@ type ionItem struct {

var binaryRoundTripSkipList = []string{
"allNulls.ion",
"bigInts.ion",
"clobWithNonAsciiCharacter.10n",
"clobs.ion",
"decimal64BitBoundary.ion",
"decimals.ion",
"float32.10n",
"floats.ion",
"intBigSize1201.10n",
"intBigSize13.10n",
"intBigSize14.10n",
"intBigSize16.10n",
"intBigSize256.10n",
"intBigSize256.ion",
"intBigSize512.ion",
"intLongMaxValuePlusOne.10n",
"localSymbolTableImportZeroMaxId.ion",
"nullDecimal.10n",
"nulls.ion",
"structWhitespace.ion",
"subfieldInt.ion",
"subfieldUInt.ion",
"subfieldVarInt.ion",
"subfieldVarUInt.ion",
"subfieldVarUInt15bit.ion",
"subfieldVarUInt16bit.ion",
"subfieldVarUInt32bit.ion",
"symbolEmpty.ion",
"symbols.ion",
"T2.10n",
"T3.10n",
"T5.10n",
"T7-large.10n",
"T9.10n",
"nullDecimal.10n",
"testfile22.ion",
"testfile23.ion",
"testfile31.ion",
"testfile35.ion",
"testfile37.ion",
"utf16.ion",
"utf32.ion",
"whitespace.ion",
"T5.10n", /*
"bigInts.ion",
"clobWithNonAsciiCharacter.10n",
"clobs.ion",
"decimal64BitBoundary.ion",
"decimals.ion",
"float32.10n",
"floats.ion",
"intBigSize1201.10n",
"intBigSize13.10n",
"intBigSize14.10n",
"intBigSize16.10n",
"intBigSize256.10n",
"intBigSize256.ion",
"intBigSize512.ion",
"intLongMaxValuePlusOne.10n",
"localSymbolTableImportZeroMaxId.ion",
"nullDecimal.10n",
"nulls.ion",
"structWhitespace.ion",
"subfieldInt.ion",
"subfieldUInt.ion",
"subfieldVarInt.ion",
"subfieldVarUInt.ion",
"subfieldVarUInt15bit.ion",
"subfieldVarUInt16bit.ion",
"subfieldVarUInt32bit.ion",
"symbolEmpty.ion",
"symbols.ion",
"T2.10n",
"T3.10n",
"T5.10n",
"T7-large.10n",
"T9.10n",
"testfile22.ion",
"testfile23.ion",
"testfile31.ion",
"testfile35.ion",
"testfile37.ion",
"utf16.ion",
"utf32.ion",
"whitespace.ion",*/
}

var textRoundTripSkipList = []string{
Expand Down Expand Up @@ -337,17 +341,24 @@ func testBinaryRoundTrip(t *testing.T, fp string) {
fileBytes := loadFile(t, fp)

// Make a binary writer from the file
binWriter, buf := encodeAsBinaryIon(t, fileBytes)
buf := encodeAsBinaryIon(t, fileBytes)

// Re-encode binWriter's stream as text into a string builder
_, str := encodeAsTextIon(t, buf.String())
str := encodeAsTextIon(t, buf.String())

reader1 := NewReader(bytes.NewReader(buf.Bytes()))
reader2 := NewReader(strings.NewReader(str.String()))

// Re-encode str as binary in another binary writer
binWriter2, _ := encodeAsBinaryIon(t, []byte(str.String()))
for reader1.Next() {
i1 := readCurrentValue(t, reader1)
reader2.Next()
i2 := readCurrentValue(t, reader2)

// Compare the 2 binary writers
if !reflect.DeepEqual(binWriter, binWriter2) {
t.Errorf("Round trip test failed on: " + fp)
if !reflect.DeepEqual(i1.value, i2.value) ||
!reflect.DeepEqual(i1.annotations, i2.annotations) ||
!reflect.DeepEqual(i1.ionType, i2.ionType) {
t.Errorf("failed %s %v vs %v", fp, i1.value, i2.value)
}
}
}

Expand All @@ -357,38 +368,45 @@ func testTextRoundTrip(t *testing.T, fp string) {
fileBytes := loadFile(t, fp)

// Make a text writer from the file
txtWriter, str := encodeAsTextIon(t, string(fileBytes))
str := encodeAsTextIon(t, string(fileBytes))

// Re-encode txtWriter's stream as binary into a bytes.Buffer
_, buf := encodeAsBinaryIon(t, []byte(str.String()))
buf := encodeAsBinaryIon(t, []byte(str.String()))

// Re-encode buf's stream as text in another text writer
txtWriter2, _ := encodeAsTextIon(t, buf.String())
reader1 := NewReader(strings.NewReader(str.String()))
reader2 := NewReader(bytes.NewReader(buf.Bytes()))

//compare the 2 text writers
if !reflect.DeepEqual(txtWriter, txtWriter2) {
t.Errorf("Round trip test failed on: " + fp)
for reader1.Next() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good. 👍

i1 := readCurrentValue(t, reader1)
reader2.Next()
i2 := readCurrentValue(t, reader2)

if !reflect.DeepEqual(i1.value, i2.value) ||
!reflect.DeepEqual(i1.annotations, i2.annotations) ||
!reflect.DeepEqual(i1.ionType, i2.ionType) {
t.Errorf("failed %s %v vs %v", fp, i1.value, i2.value)
}
}
}

// Create a TextWriter from data parameter. Return the writer and string builder containing writer's contents
Copy link
Contributor

Choose a reason for hiding this comment

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

These function comments refer to the old return types.

func encodeAsTextIon(t *testing.T, data string) (Writer, strings.Builder) {
func encodeAsTextIon(t *testing.T, data string) strings.Builder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Golang allows you to use the string data type to mean "arbitrary bytes", but I don't think we should use it that way. When I first read this function I was surprised because I thought if data was a string, it must already be text Ion so there'd be no need to encode it as text Ion. Only when I went and looked at the callsite did I realize that we were taking a binary buffer and calling String() on it to get a string instance.

We should make sure we're only using string to refer to text throughout the APIs. If this is a quick change, you can include it in the PR. Otherwise, please open an issue so we don't forget to address it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it in this test, create #48 to check throughout the project.

reader := NewReader(strings.NewReader(data))
str := strings.Builder{}
txtWriter := NewTextWriter(&str)
writeToWriterFromReader(t, reader, txtWriter)
txtWriter.Finish()
Copy link
Contributor

Choose a reason for hiding this comment

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

Only caught this through linting, I'm getting a few warnings with unhandled exceptions.

Been looking through best practices for error handling in tests and it seems we can just make this obvious if we're ignoring an error by doing _ = txtWriter.Finish().

Looking into what the best practices are, but I'd feel failing early when an error occurred would be good. Let says if the text writer failed to finish, would this be obvious if we continued to ignoring the error and let the test fail later on? But, this could be a unit test vs integration test scenario, so maybe explicity ignoring the error for these cases is good enough.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I did not put error handling for writer.Finish(), stepIn() and stepOut().
My approach was to test the testfiles against different scenarios of this file (roundtrip, equivalency, loadBad). And if it fails at any point, the test fails. But open to suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

My general preference would be to explicitly check for errors from the writer and t.Fatal(err); if something goes wrong during writing, that'll give you more information about the problem than one of the outputs being truncated.

The writers should remember any errors they encounter along the way, no-op future writes, and return the error from Finish, so in theory you can get away with just checking the return value there. (Although that's maybe something that's worth testing more explicitly than it is tested now. :))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return txtWriter, str
return str
}

// Create a BinaryWriter from data parameter. Return the writer and buffer containing writer's contents
func encodeAsBinaryIon(t *testing.T, data []byte) (Writer, bytes.Buffer) {
func encodeAsBinaryIon(t *testing.T, data []byte) bytes.Buffer {
reader := NewReader(bytes.NewReader(data))
buf2 := bytes.Buffer{}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like buf2 could be named buf?

binWriter2 := NewBinaryWriter(&buf2)
writeToWriterFromReader(t, reader, binWriter2)
binWriter2.Finish()
return binWriter2, buf2
return buf2
}

// Execute loading malformed Ion values into a Reader and validate the Reader.
Expand Down