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

Test/add api tests #20

Merged
merged 5 commits into from
Nov 28, 2023
Merged

Test/add api tests #20

merged 5 commits into from
Nov 28, 2023

Conversation

alexis-langlet
Copy link
Contributor

Test part of api

@alexis-langlet alexis-langlet self-assigned this Nov 27, 2023
WoodenMaiden
WoodenMaiden approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@WoodenMaiden WoodenMaiden left a comment

Choose a reason for hiding this comment

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

Besides this this PR looks good 👍

api/Cargo.toml Show resolved Hide resolved
lucido-simon
lucido-simon requested changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@lucido-simon lucido-simon left a comment

Choose a reason for hiding this comment

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

Overall, LGTM, excepts a few nits.

I'm worried we are adding too much complexity to the code just to be able to test it, but I unfortunately don't see any other way of doing it.

api/src/api/service.rs Show resolved Hide resolved
api/Cargo.toml Show resolved Hide resolved
api/src/vm_manager/mod.rs Outdated Show resolved Hide resolved
@alexis-langlet
test(api): add mockall and automock VMManagerTrait
ba54bec 
Define a trait that is implemented by VMManger for testing purpose
also refactor part of vm_manager in the meantime
@alexis-langlet
test(api): test run_code in service
0b1964d
@alexis-langlet
test(api): test run_code in mod
600982f
lucido-simon
lucido-simon approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@lucido-simon lucido-simon left a comment

Choose a reason for hiding this comment

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

LGTM

@alexis-langlet alexis-langlet merged commit d24989b into main Nov 28, 2023
4 checks passed
@alexis-langlet alexis-langlet deleted the test/add-api-tests branch November 28, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WoodenMaiden WoodenMaiden WoodenMaiden approved these changes

@lucido-simon lucido-simon lucido-simon approved these changes

Assignees

@alexis-langlet alexis-langlet

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexis-langlet @lucido-simon @WoodenMaiden