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

Allow for assertions against intentional non-match #705

Merged
merged 11 commits into from
Jun 3, 2022

Conversation

Will-Sommers
Copy link
Collaborator

@Will-Sommers Will-Sommers commented May 28, 2022

What?

This PR adds the ability to capture errors and non-matches from issued commands in the integration tests.

Why?

I wasn't able to do this as part of the TS-Scheme query PR and so added unit tests. I think it would be nice to have both. Non-matches help describe a complete spec of how expected behavior and unexpected behavior should be handled.

How?

  • Add a new VSCode command to capture error cases. This is "off" by default.
  • Note if an error is thrown and pass it to the TestCaseRecorder
  • Do not record finalState instead record errorReturned as a boolean
  • If errorReturned is present on the fixture, ensure we throw

Todo

  • Make a new test folder to assert on these errors
  • Make errorReturned richer? I don't want to assert on an error message but I wonder if there is information that we can record to make the test cases more valuable.

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Great stuff! Will be really nice to be able to record this type of test. Left a bunch of minor comments

package.json Outdated Show resolved Hide resolved
src/core/commandRunner/CommandRunner.ts Outdated Show resolved Hide resolved
src/test/suite/fixtures/recorded/actions/takeList.yml Outdated Show resolved Hide resolved
src/test/suite/fixtures/recorded/actions/takeList2.yml Outdated Show resolved Hide resolved
src/testUtil/TestCase.ts Outdated Show resolved Hide resolved
src/testUtil/TestCase.ts Outdated Show resolved Hide resolved
src/testUtil/TestCase.ts Outdated Show resolved Hide resolved
src/testUtil/TestCase.ts Outdated Show resolved Hide resolved
src/testUtil/TestCase.ts Outdated Show resolved Hide resolved
src/testUtil/TestCaseRecorder.ts Outdated Show resolved Hide resolved
@Will-Sommers Will-Sommers requested a review from pokey May 31, 2022 18:08
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Almost there! Left another round of feedback

src/test/suite/recorded.test.ts Outdated Show resolved Hide resolved
src/testUtil/TestCase.ts Outdated Show resolved Hide resolved
src/testUtil/TestCase.ts Outdated Show resolved Hide resolved
src/testUtil/TestCaseRecorder.ts Outdated Show resolved Hide resolved
src/testUtil/TestCaseRecorder.ts Outdated Show resolved Hide resolved
src/testUtil/TestCaseRecorder.ts Outdated Show resolved Hide resolved
docs/contributing/test-case-recorder.md Outdated Show resolved Hide resolved
@Will-Sommers Will-Sommers requested a review from pokey May 31, 2022 20:29
@pokey pokey mentioned this pull request Jun 1, 2022
8 tasks
/** The final state after a command is issued. Undefined if we are testing a non-match(error) case. */
finalState?: TestCaseSnapshot;
/** Used to assert if an error has been thrown. */
errorName?: ThrownError;
Copy link
Member

Choose a reason for hiding this comment

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

rename to thrownError

@@ -248,7 +257,6 @@ export class TestCaseRecorder {
// command for a navigation map test
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

re-add

pokey
pokey previously approved these changes Jun 3, 2022
@pokey pokey merged commit 87df330 into main Jun 3, 2022
@pokey pokey deleted the wsommers/error-recorder branch June 3, 2022 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants