refactor: Migrate gql introspection tests to new framework #1211

AndrewSisley · 2023-03-21T20:39:46Z

Relevant issue(s)

Resolves #1210

Description

Migrates the gql introspection tests to the new test framework.

I need to introspect the result of patch schema soon, and migrating the introspection framework to the new main framework seemed a much better option than hacking it into the existing. The main framework should now be capable of handling this.

I also tweaked the logic for flattening action groups recently added, the reasons for doing so are documented in the commit body.

The tests are almost identical to in develop, with the vast majority of tests just being copy-pasted into the new data structure. There are a couple of exceptions in the commit Migrate simple_test.go to new framework regarding expected errors - the details are noted in the commit body.

All documentation has been copied pasted from the original.

I tested a handful of tests to ensure they were still executing correctly by tweaking the expected values and watching them fail.

Suggest reviewing commit by commit, but it is probs not worth really reading most of the Migrate foo_test.go... commits minus perhaps the exception noted above as they are just copy-pastes of large introspection query blocks.

The code is shorter but more complex. Importantly it means that we do not need to remember to add new action types to 3 code locations (len calc, flatten, main-loop) within the framework, something that is error prone and a bit of a hassle.

Code is copy-pasted from existing schema/utils.go file, plus a couple of compilation tweaks and var renames.

No changes to test content

No changes to test contents

Due to differences in error handling some of these tests have changed slightly. In the old introspection tests, `ExpectedErrrors` would be compared against errrors generated during schema update, and during introspection - the new framework ties an expected error to a specific action. For these tests it is now clear that the errors occur on schema update, and not during introspection. This means that in the original, the introspection query was declared but never actually run (test would exist early on expected error), as such this commit also removes the declaration of these never-executed introspection queries.

No changes to test contents

source-devs · 2023-03-21T20:48:40Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

fredcarle

LGTM. Just one todo and a question. I assume the todo will be taken care of before merge.

fredcarle · 2023-03-21T20:54:01Z

tests/integration/utils2.go

@@ -1131,3 +1099,67 @@ func assertExpectedErrorRaised(t *testing.T, description string, expectedError s
 		assert.Fail(t, "Expected an error however none was raised.", description)
 	}
 }
+
+func assertSchemaResults(


todo: That name is a bit misleading. Maybe assertIntrospectionResults?

Good spot, cheers Fred

Rename assertIntrospectionResults

fredcarle · 2023-03-21T20:57:22Z

tests/integration/utils2.go

+func assertContains(t *testing.T, contains map[string]any, actual map[string]any) {
+	for k, expected := range contains {
+		innerActual := actual[k]
+		if innerExpected, innerIsMap := expected.(map[string]any); innerIsMap {


question: Any reason for not using a switch statement instead?

Cant say for sure as this was copy pasted, but might have grown organically and the difference is pretty minimal to me here still.

source-devs · 2023-03-21T21:18:20Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

shahzadlone

Mostly looks good, pointed out an unnecessary panic that will be caused that can be easily fixed (but this is probably an issue with most other test cases as well).

shahzadlone · 2023-03-21T21:19:31Z

tests/integration/utils2.go

+	if AssertErrors(t, description, result.GQL.Errors, action.ExpectedError) {
+		return true
+	}
+	resultantData := result.GQL.Data.(map[string]any)
+
+	if len(action.ExpectedData) == 0 && len(action.ContainsData) == 0 {
+		assert.Equal(t, action.ExpectedData, resultantData)
+	}
+
+	if len(action.ExpectedData) == 0 && len(action.ContainsData) > 0 {
+		assertContains(t, action.ContainsData, resultantData)
+	} else {
+		assert.Equal(t, len(action.ExpectedData), len(resultantData))
+
+		for k, result := range resultantData {
+			assert.Equal(t, action.ExpectedData[k], result)


todo: There will be some unnecessary panics here if the expected result is wrongly entered to be say: ExpectedData: map[string]any{} and actual result is > 0.

Using require over assert incases where we can terminate early fixes this (found a mismatch, just exit) .

An example of this I fixed for another test case is in this commit: 1b26794

Ah, I dont particularly care about that right now. This is copy-paste code, and a panic is about as good as an assert failure to me here. If I spent more time thinking about this (which I dont want to right now) there are probably very good odds I would rather keep the test code simple and live with the panics instead of complicating things for the sake of turning a panic into an assert failure.

Will make an issue to resolve this then when I convert the new explain testing setup to the new integration setup.

EDIT: Nvm I see you are doing the change. Thanks

codecov · 2023-03-21T21:29:12Z

Codecov Report

Merging #1211 (8877139) into develop (990fad8) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1211   +/-   ##
========================================
  Coverage    68.65%   68.65%           
========================================
  Files          181      181           
  Lines        17125    17125           
========================================
+ Hits         11757    11758    +1     
+ Misses        4414     4413    -1     
  Partials       954      954

see 1 file with indirect coverage changes

shahzadlone

Thanks for doing the todo :) and taking the time to convert these.

source-devs · 2023-03-21T21:40:07Z

Benchmark Results

Summary

0 Benchmarks successfully compared.
0 Benchmarks were ✅ Better.
0 Benchmarks were ❌ Worse .
0 Benchmarks were ✨ Unchanged.

✅ See Better Results...

	time/op		delta

❌ See Worse Results...

	time/op		delta

✨ See Unchanged Results...

	time/op		delta

🐋 See Full Results...

* Use reflect for action bundling The code is shorter but more complex. Importantly it means that we do not need to remember to add new action types to 3 code locations (len calc, flatten, main-loop) within the framework, something that is error prone and a bit of a hassle. * Add gql introspection support to test framework Code is copy-pasted from existing schema/utils.go file, plus a couple of compilation tweaks and var renames. * Migrate client_test.go to new framework No changes to test content * Migrate filter_test.go to new framework No changes to test contents * Migrate input_type_test.go to new framework No changes to test contents * Migrate simple_test.go to new framework Due to differences in error handling some of these tests have changed slightly. In the old introspection tests, `ExpectedErrrors` would be compared against errrors generated during schema update, and during introspection - the new framework ties an expected error to a specific action. For these tests it is now clear that the errors occur on schema update, and not during introspection. This means that in the original, the introspection query was declared but never actually run (test would exist early on expected error), as such this commit also removes the declaration of these never-executed introspection queries. * Migrate with_inline_array_test.go to new framework No changes to test contents * Migrate inline_array_test.go to new framework No changes to test contents * Migrate simple_test.go to new framework No changes to test contents * Migrate top_level_test.go to new framework No changes to test contents * Remove old test utils

…work#1211) * Use reflect for action bundling The code is shorter but more complex. Importantly it means that we do not need to remember to add new action types to 3 code locations (len calc, flatten, main-loop) within the framework, something that is error prone and a bit of a hassle. * Add gql introspection support to test framework Code is copy-pasted from existing schema/utils.go file, plus a couple of compilation tweaks and var renames. * Migrate client_test.go to new framework No changes to test content * Migrate filter_test.go to new framework No changes to test contents * Migrate input_type_test.go to new framework No changes to test contents * Migrate simple_test.go to new framework Due to differences in error handling some of these tests have changed slightly. In the old introspection tests, `ExpectedErrrors` would be compared against errrors generated during schema update, and during introspection - the new framework ties an expected error to a specific action. For these tests it is now clear that the errors occur on schema update, and not during introspection. This means that in the original, the introspection query was declared but never actually run (test would exist early on expected error), as such this commit also removes the declaration of these never-executed introspection queries. * Migrate with_inline_array_test.go to new framework No changes to test contents * Migrate inline_array_test.go to new framework No changes to test contents * Migrate simple_test.go to new framework No changes to test contents * Migrate top_level_test.go to new framework No changes to test contents * Remove old test utils

AndrewSisley added 11 commits March 21, 2023 15:37

Use reflect for action bundling

1021221

The code is shorter but more complex. Importantly it means that we do not need to remember to add new action types to 3 code locations (len calc, flatten, main-loop) within the framework, something that is error prone and a bit of a hassle.

Add gql introspection support to test framework

da09f42

Code is copy-pasted from existing schema/utils.go file, plus a couple of compilation tweaks and var renames.

Migrate client_test.go to new framework

9ec21c9

No changes to test content

Migrate filter_test.go to new framework

cbf298f

No changes to test contents

Migrate input_type_test.go to new framework

b4e0e7c

No changes to test contents

Migrate with_inline_array_test.go to new framework

f6ddde6

No changes to test contents

Migrate inline_array_test.go to new framework

6eb910a

No changes to test contents

Migrate simple_test.go to new framework

a0b0698

No changes to test contents

Migrate top_level_test.go to new framework

b2a608b

No changes to test contents

Remove old test utils

5014399

AndrewSisley added area/schema Related to the schema system area/testing Related to any test or testing suite refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Mar 21, 2023

AndrewSisley added this to the DefraDB v0.5 milestone Mar 21, 2023

AndrewSisley requested a review from a team March 21, 2023 20:39

AndrewSisley self-assigned this Mar 21, 2023

fredcarle approved these changes Mar 21, 2023

View reviewed changes

PR FIXUP - Rename assertIntrospectionResults

ff42eb2

shahzadlone requested changes Mar 21, 2023

View reviewed changes

PR FIXUP - require instead of assert

8877139

shahzadlone approved these changes Mar 21, 2023

View reviewed changes

AndrewSisley merged commit f0e2036 into develop Mar 21, 2023

AndrewSisley deleted the sisley/refactor/I1210-migrate-gql-introspection-tests branch March 21, 2023 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Migrate gql introspection tests to new framework #1211

refactor: Migrate gql introspection tests to new framework #1211

AndrewSisley commented Mar 21, 2023

source-devs commented Mar 21, 2023

fredcarle left a comment

fredcarle Mar 21, 2023

AndrewSisley Mar 21, 2023 •

edited

Loading

fredcarle Mar 21, 2023

AndrewSisley Mar 21, 2023

source-devs commented Mar 21, 2023

shahzadlone left a comment

shahzadlone Mar 21, 2023

AndrewSisley Mar 21, 2023 •

edited

Loading

shahzadlone Mar 21, 2023 •

edited

Loading

AndrewSisley Mar 21, 2023

codecov bot commented Mar 21, 2023 •

edited

Loading

shahzadlone left a comment

source-devs commented Mar 21, 2023

refactor: Migrate gql introspection tests to new framework #1211

refactor: Migrate gql introspection tests to new framework #1211

Conversation

AndrewSisley commented Mar 21, 2023

Relevant issue(s)

Description

source-devs commented Mar 21, 2023

Benchmark Results

Summary

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle Mar 21, 2023

Choose a reason for hiding this comment

AndrewSisley Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

fredcarle Mar 21, 2023

Choose a reason for hiding this comment

AndrewSisley Mar 21, 2023

Choose a reason for hiding this comment

source-devs commented Mar 21, 2023

Benchmark Results

Summary

shahzadlone left a comment

Choose a reason for hiding this comment

shahzadlone Mar 21, 2023

Choose a reason for hiding this comment

AndrewSisley Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

shahzadlone Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

AndrewSisley Mar 21, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 21, 2023 • edited Loading

Codecov Report

shahzadlone left a comment

Choose a reason for hiding this comment

source-devs commented Mar 21, 2023

Benchmark Results

Summary

AndrewSisley Mar 21, 2023 •

edited

Loading

AndrewSisley Mar 21, 2023 •

edited

Loading

shahzadlone Mar 21, 2023 •

edited

Loading

codecov bot commented Mar 21, 2023 •

edited

Loading