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.

Sign up for GitHub

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

Jump to bottom

initial testing of fsm #14

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

initial testing of fsm #14

wants to merge 1 commit into from

Conversation

yusufadam2
Copy link
Contributor

No description provided.

AndrewF001
AndrewF001 approved these changes Apr 14, 2023
View reviewed changes
Copy link
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()

crates/analyzer-host/src/fsm.rs Outdated Show resolved Hide resolved
crates/analyzer-host/src/fsm.rs Outdated Show resolved Hide resolved
viluon
viluon reviewed Apr 18, 2023
View reviewed changes
Copy link
Member

@viluon viluon left a comment •
edited
Loading

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.

crates/analyzer-host/src/fsm.rs Outdated Show resolved Hide resolved
crates/analyzer-host/src/fsm.rs Outdated Show resolved Hide resolved
@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
viluon
viluon approved these changes May 12, 2023
View reviewed changes
Copy link
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.

crates/analyzer-host/src/tests/unit_tests.rs Outdated Show resolved Hide resolved
AndrewF001
AndrewF001 requested changes May 15, 2023
View reviewed changes
crates/analyzer-host/src/tests/unit_tests.rs Outdated Show resolved Hide resolved
crates/analyzer-host/src/tests/unit_tests.rs Outdated Show resolved Hide resolved
crates/analyzer-host/src/tests/unit_tests.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewF001 AndrewF001 AndrewF001 requested changes

@viluon viluon viluon approved these changes

@timjroberts timjroberts Awaiting requested review from timjroberts

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yusufadam2 @viluon @AndrewF001