Skip to content

initial testing of fsm#14

Open
yusufadam2 wants to merge 1 commit intomainfrom
ya/fsm_tests
Open

initial testing of fsm#14
yusufadam2 wants to merge 1 commit intomainfrom
ya/fsm_tests

Conversation

@yusufadam2
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@AndrewF001 AndrewF001 left a comment

Choose a reason for hiding this comment

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

LGTM! Like the work on test_process_message()

Comment thread crates/analyzer-host/src/fsm.rs Outdated
Comment thread crates/analyzer-host/src/fsm.rs Outdated
Copy link
Copy Markdown
Member

@viluon viluon left a comment

Choose a reason for hiding this comment

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

This looks generally good, although the tests only exercise the "happy path." I think a variant of test_process_message() that sends invalid input (like two initialize requests in a row) would have a higher chance of catching bugs.

Comment thread crates/analyzer-host/src/fsm.rs Outdated
Comment thread crates/analyzer-host/src/fsm.rs Outdated
@yusufadam2 yusufadam2 force-pushed the ya/fsm_tests branch 8 times, most recently from 2a0639b to 87987e7 Compare May 11, 2023 16:46
@viluon viluon self-requested a review May 12, 2023 11:45
Copy link
Copy Markdown
Member

@viluon viluon left a comment

Choose a reason for hiding this comment

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

This looks good but you might break the build of main now that we have the lockfile in place (due to the new dependency). Merge main into your branch before merging the PR, that should lead to a CI failure which you can then fix by committing your updated Cargo.lock.

Comment thread crates/analyzer-host/src/tests/unit_tests.rs Outdated
Comment thread crates/analyzer-host/src/tests/unit_tests.rs Outdated
Comment thread crates/analyzer-host/src/tests/unit_tests.rs Outdated
Comment thread crates/analyzer-host/src/tests/unit_tests.rs Outdated
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.

3 participants