Add integration tests with skip lists for failing tests #33

R-maan · 2020-05-20T19:29:29Z

This is integration test with ion-test repository.
This file contains the following tests:

Roundtrip binary to text and back to binary again
Roundtrip text to binary and back to text again
Load bad files (malformed ion files in /bad directory)
Equivalency test
Non-equivalency test

Each of the tests have skip list for the tests which are currently failing.
In these tests, low level APIs have been used.

codecov-commenter · 2020-05-20T19:35:26Z

Codecov Report

Merging #33 into master will increase coverage by 4.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
+ Coverage   71.83%   75.83%   +4.00%     
==========================================
  Files          22       22              
  Lines        4573     4573              
==========================================
+ Hits         3285     3468     +183     
+ Misses        797      681     -116     
+ Partials      491      424      -67

Impacted Files	Coverage Δ
ion/binaryreader.go	`80.27% <0.00%> (+3.40%)`	⬆️
ion/textutils.go	`78.19% <0.00%> (+4.73%)`	⬆️
ion/bitstream.go	`78.03% <0.00%> (+7.11%)`	⬆️
ion/tokenizer.go	`79.24% <0.00%> (+8.54%)`	⬆️
ion/textreader.go	`85.64% <0.00%> (+15.11%)`	⬆️
ion/err.go	`62.50% <0.00%> (+37.50%)`	⬆️

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 30f3e38...70845d0. Read the comment docs.

fernomac · 2020-05-20T22:59:06Z

Nice! These are on track to be a lot more comprehensive than the tests in reader_test.go and should probably replace those.

ion/integration_test.go

zslayton · 2020-05-26T12:45:12Z

ion/integration_test.go

+	binWriter2, _ := makeBinaryWiter(t, []byte(str.String()))
+
+	// Compare the 2 binary writers
+	if !reflect.DeepEqual(binWriter, binWriter2) {


Based on my reading of the docs for reflect#deepEquals, it looks like we're doing a structural comparison of the Writer interface values that were created by makeBinaryWriter. We should be evaluating the equivalence of the streams that were produced by the writers and not the final state of the writers themselves. Typically that's done by creating a Reader for each stream and deeply comparing the values they encounter for equality.

Are you suggesting to change the comparison between buf and the buffer for binWriter2?
I tested it and the results are different from current comparison.

Per our offline discussion, we want to compare the values that can be read from each encoding rather than comparing the Writers or buffers directly.

ion/integration_test.go

matthewh-BQ · 2020-05-26T20:52:01Z

ion/integration_test.go

+	str := strings.Builder{}
+	txtWriter := NewTextWriter(&str)
+	writeToWriterFromReader(t, reader, txtWriter)
+	txtWriter.Finish()


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?

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.

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. :))

ion/integration_test.go

zslayton · 2020-05-27T17:26:11Z

ion/integration_test.go

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


This looks good. 👍

zslayton · 2020-05-27T17:36:11Z

ion/integration_test.go

-		}
+	buf2 := encodeAsBinaryIon(t, []byte(str.String()))
+
+	if !reflect.DeepEqual(buf, buf2) {


We don't want to compare the buffers or strings that are produced directly; the encoded bytes can be different but still contain equivalent values. For example, the TextWriter and the PrettyTextWriter will produce different strings, but the values in the strings should be considered the same. Similarly, some data types like Timestamp have a variety of ways to encode the same value; 2007-02-23T00:00Z and 2007-02-23T00:00+00:00 are equivalent despite being encoded differently.

I was hoping you read the email I sent about this before seeing it in the PR :P
Updated the test as talked offline.

zslayton

Just a few small cleanup items remaining.

zslayton · 2020-05-28T19:14:35Z

ion/integration_test.go

+	}
+}
+
+// Execute round trip testing for TextWriter. Create a TextWriter, using the TextWriter creat a


It looks like these comments are out of date now. ("Validate that the first and last Writers are equal.")

"creat" -> "create"

zslayton · 2020-05-28T19:18:08Z

ion/integration_test.go

+	}
+}
+
+// Create a TextWriter from data parameter. Return the writer and string builder containing writer's contents


These function comments refer to the old return types.

zslayton · 2020-05-28T19:19:16Z

ion/integration_test.go

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


It looks like buf2 could be named buf?

zslayton

I've added some suggested phrasings for the new function comments. I've also got one code change to request, but we can open an issue to track it if needed.

zslayton · 2020-05-29T14:28:03Z

ion/integration_test.go

+// Execute round trip testing for the BinaryWriter by creating a BinaryWriter and re-encoding it to text.
+// From the output of the writers, construct two readers and verify the equivalency of both readers.


// Re-encodes the provided file as binary Ion, reads that binary Ion and writes it as text Ion, // then constructs Readers over the binary and text encodings to verify that the streams // are equivalent.

zslayton · 2020-05-29T14:29:03Z

ion/integration_test.go

+// Execute round trip testing for the TextWriter by creating a TextWriter and re-encoding it to binary.
+// From the output of the writers, construct two readers and verify the equivalency of both readers.


// Re-encodes the provided file as text Ion, reads that text Ion and writes it as binary Ion, // then constructs Readers over the text and binary encodings to verify that the streams // are equivalent.

zslayton · 2020-05-29T14:38:57Z

ion/integration_test.go

@@ -395,7 +395,7 @@ func testTextRoundTrip(t *testing.T, fp string) {
 	}
 }

-// Create a TextWriter from data parameter. Return the writer and string builder containing writer's contents
+// Create a TextWriter from data parameter and return the string builder containing writer's contents.


Now that we're not returning the TextWriter, it's an implementation detail.

// Re-encode the provided Ion data as a text Ion string.

zslayton · 2020-05-29T14:39:07Z

ion/integration_test.go

@@ -408,17 +408,17 @@ func encodeAsTextIon(t *testing.T, data string) strings.Builder {
 	return str
 }

-// Create a BinaryWriter from data parameter. Return the writer and buffer containing writer's contents
+// Create a BinaryWriter from data parameter and return the buffer containing writer's contents.


Now that we're not returning the BinaryWriter, it's an implementation detail.

// Re-encode the provided Ion data as a binary Ion buffer.

zslayton · 2020-05-29T14:42:34Z

ion/integration_test.go

@@ -395,7 +395,7 @@ func testTextRoundTrip(t *testing.T, fp string) {
 	}
 }

-// Create a TextWriter from data parameter. Return the writer and string builder containing writer's contents
+// Create a TextWriter from data parameter and return the string builder containing writer's contents.
 func encodeAsTextIon(t *testing.T, data string) strings.Builder {


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.

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

R-maan · 2020-05-29T17:00:30Z

Updated the PR with suggested changes

Add integration tests with skip lists for failing tests

10543fd

zslayton requested changes May 21, 2020

View reviewed changes

Arman A added 2 commits May 21, 2020 22:17

Increase code readability, change regex with HasSuffix

b80c6f9

Refactor round trip tests

db465ed

R-maan requested a review from zslayton May 22, 2020 16:23

zslayton requested changes May 26, 2020

View reviewed changes

Updating comments, messages, renaming methods and variables,

d8e8816

R-maan requested a review from zslayton May 26, 2020 19:02

matthewh-BQ reviewed May 26, 2020

View reviewed changes

zslayton reviewed May 26, 2020

View reviewed changes

ion/integration_test.go Outdated Show resolved Hide resolved

R-maan mentioned this pull request May 26, 2020

Move equivalency test assertions #45

Closed

matthewh-BQ reviewed May 26, 2020

View reviewed changes

ion/integration_test.go Outdated Show resolved Hide resolved

ion/integration_test.go Outdated Show resolved Hide resolved

R-maan mentioned this pull request May 26, 2020

Define a type to represent any Ion Value #46

Open

Arman A added 2 commits May 27, 2020 09:34

Round trip test logic change

435dba9

Round trip test logic change option 2

ae74409

zslayton reviewed May 27, 2020

View reviewed changes

ion/integration_test.go Outdated Show resolved Hide resolved

zslayton reviewed May 27, 2020

View reviewed changes

Arman A added 3 commits May 27, 2020 11:14

Improve Round trip test - update skip list - refactor

534d221

Update skip list

40ed246

Handle Type.null - add error handling - update skip lists

71c5d5a

R-maan requested a review from zslayton May 27, 2020 22:39

zslayton requested changes May 28, 2020

View reviewed changes

Update comments

f9bff48

R-maan requested a review from zslayton May 28, 2020 21:57

zslayton requested changes May 29, 2020

View reviewed changes

Update comments - change signature of a helper function

b1e2c09

R-maan mentioned this pull request May 29, 2020

Using 'string' only to refer to text throughout the APIs #48

Closed

Merge branch 'master' into integration-test

70845d0

zslayton approved these changes May 29, 2020

View reviewed changes

zslayton merged commit c6d91a2 into amazon-ion:master May 29, 2020

R-maan added a commit to R-maan/ion-go that referenced this pull request Jun 1, 2020

Add integration tests with skip lists for failing tests (amazon-ion#33)

ae54ec4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration tests with skip lists for failing tests #33

Add integration tests with skip lists for failing tests #33

R-maan commented May 20, 2020

codecov-commenter commented May 20, 2020 •

edited

Loading

fernomac commented May 20, 2020

zslayton May 26, 2020

R-maan May 26, 2020

zslayton May 26, 2020

R-maan May 27, 2020

matthewh-BQ May 26, 2020

R-maan May 26, 2020

fernomac May 26, 2020

R-maan May 27, 2020

zslayton May 27, 2020

zslayton May 27, 2020

R-maan May 27, 2020 •

edited

Loading

zslayton left a comment

zslayton May 28, 2020

zslayton May 28, 2020

zslayton May 28, 2020

zslayton left a comment

zslayton May 29, 2020

zslayton May 29, 2020

zslayton May 29, 2020

zslayton May 29, 2020

zslayton May 29, 2020

R-maan May 29, 2020

R-maan commented May 29, 2020

		// Execute round trip testing for the BinaryWriter by creating a BinaryWriter and re-encoding it to text.
		// From the output of the writers, construct two readers and verify the equivalency of both readers.

		// Execute round trip testing for the TextWriter by creating a TextWriter and re-encoding it to binary.
		// From the output of the writers, construct two readers and verify the equivalency of both readers.

Add integration tests with skip lists for failing tests #33

Add integration tests with skip lists for failing tests #33

Conversation

R-maan commented May 20, 2020

codecov-commenter commented May 20, 2020 • edited Loading

Codecov Report

fernomac commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R-maan May 27, 2020 • edited Loading

Choose a reason for hiding this comment

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R-maan commented May 29, 2020

codecov-commenter commented May 20, 2020 •

edited

Loading

R-maan May 27, 2020 •

edited

Loading