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

test(i): Integrate explain tests into test action system #1545

Conversation

shahzadlone
Copy link
Member

@shahzadlone shahzadlone commented May 29, 2023

Resolves #1243

Description:

  • Integrates the new explain test setup with the new testing with action based setup.
  • Converts all simple explain tests.
  • Converts all default explain tests.
  • Converts all execute explain tests.
  • Removes the documents for default and simple tests as they aren't worth maintaining (it doesn't effect simple explain testing).
  • Fixed a test that was being omitted due to it's file not having the extension "*_test.go".

For Reviewers:

  • I believe commit by commit review should be fairly clean.
  • Mostly concerned about the commit(s) with (CORE) commit label decorator.
  • Commits with (RENAME) or (UNRELATED) label decorators can be ignored.
  • Commits with(PREP) label decorators are preparations needed before cleanly doing the integrations/conversions.
  • Is there any preference to moving the test cases to the test_cases.go utility files?
  • Perhaps soon we can have some sort of assertion package to store all the testing assertions as if I used the utils2.go file instead of explain.go file to put the assertion code, would have ended up with with 1700-1800 lines of code in that file, which I am not a big fan of.

How has this been tested?

I have tried many permutations of manually inputting incorrect result to ensure tests don't automatically pass.

Not sure why all the actions list was being passed when only description
is used, in the `executeRequest` function.
expected error and actual error is a mismatch, this fixed it.
@shahzadlone shahzadlone added area/testing Related to any test or testing suite code quality Related to improving code quality labels May 29, 2023
@shahzadlone shahzadlone added this to the DefraDB v0.6 milestone May 29, 2023
@shahzadlone shahzadlone self-assigned this May 29, 2023
@shahzadlone shahzadlone force-pushed the tests/integrate-explain-tests-as-test-action branch from 8b406c5 to c6c8d35 Compare May 29, 2023 19:54
@AndrewSisley
Copy link
Contributor

Perhaps soon we can have some sort of assertion package to store all the testing assertions

This could be nice

@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Merging #1545 (afcb0ee) into develop (d19bf85) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1545      +/-   ##
===========================================
+ Coverage    72.27%   72.30%   +0.03%     
===========================================
  Files          185      185              
  Lines        18374    18374              
===========================================
+ Hits         13280    13286       +6     
+ Misses        4052     4048       -4     
+ Partials      1042     1040       -2     

see 3 files with indirect coverage changes

@shahzadlone shahzadlone force-pushed the tests/integrate-explain-tests-as-test-action branch from c6c8d35 to 17ba1fa Compare May 29, 2023 21:51
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Looks good to me, good job getting all this sorted!

@shahzadlone
Copy link
Member Author

Looks good to me, good job getting all this sorted!

Speedy, I didn't even ping any reviewers yet haha.

BTW I think the new integration runs change detection on explain (and fails). I forgot if turning it off previously was intentional.

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

I like to see more lines removed than added 🙌

@AndrewSisley
Copy link
Contributor

Speedy, I didn't even ping any reviewers yet haha.

I thought you had - I got an email notifiication lol

BTW I think the new integration runs change detection on explain (and fails). I forgot if turning it off previously was intentional.

They should pass, I was having a look as to why they are failing

are not skipped now, so document to fix the breaking change detector.
@shahzadlone
Copy link
Member Author

I thought you had - I got an email notifiication lol

LOL odd, I don't think I did. But worked for the best :)

They should pass, I was having a look as to why they are failing

Documented, this is probably because explain tests didn't run with change detection before (were skipped).

@shahzadlone shahzadlone requested a review from a team May 29, 2023 23:57
@shahzadlone shahzadlone merged commit 43857bb into sourcenetwork:develop May 30, 2023
@shahzadlone shahzadlone deleted the tests/integrate-explain-tests-as-test-action branch May 30, 2023 00:02
shahzadlone added a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…rk#1545)

Resolves sourcenetwork#1243

## Description:
- [x] Integrates the new explain test setup with the new testing with
action based setup.
- [x] Converts all simple explain tests.
- [x] Converts all default explain tests.
- [x] Converts all execute explain tests.
- [x] Removes the documents for default and simple tests as they aren't
worth maintaining (it doesn't effect simple explain testing).
- [x] Fixed a test that was being omitted due to it's file not having
the extension "*_test.go".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Related to any test or testing suite code quality Related to improving code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate the new explain test setup into the new test action system
3 participants