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

Add Unittests for different components - [merged] #57

Closed
as6520 opened this issue Mar 5, 2021 · 26 comments
Closed

Add Unittests for different components - [merged] #57

as6520 opened this issue Mar 5, 2021 · 26 comments

Comments

@as6520
Copy link
Contributor

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 13:48

Merges add-tests -> master

This MR adds unittests for different components of the package. The tests are run using pytests in CI.

Closes #15, #21

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 13:52

added 1 commit

Compare with previous version

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 14:15

added 1 commit

Compare with previous version

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:06

added 5 commits

  • 3daffb4 - Add fixtures to conftest
  • 31704ac - Fix formatting error in conftest
  • c4e2e03 - Move conftest in tests directory
  • 61314c1 - Remove unused imports
  • 2f46b1c - Fix linting issues

Compare with previous version

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:08

unmarked as a Work In Progress

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:08

changed title from {-WIP: Add tes-}ts to {+Add Unittests for different componen+}ts

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:08

changed the description

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:50

added 1 commit

  • 3976379 - Add badges for pipeline and coverage

Compare with previous version

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:50

@waxlamp please review

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:50

@awk11 please review

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:53

changed the description

1 similar comment
@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 2, 2020, 15:53

changed the description

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @waxlamp on Sep 3, 2020, 11:12

Commented on tests/test_parinterface.py line 258

Don't you need an assert statement to verify that the termination occurred?

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @waxlamp on Sep 3, 2020, 11:12

Looks good overall.

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @waxlamp on Sep 3, 2020, 11:12

approved this merge request

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 3, 2020, 13:14

Commented on tests/test_parinterface.py line 258

Baseed on the function signature. I am not sure what the assertion would be upon

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @waxlamp on Sep 3, 2020, 13:53

Commented on tests/test_parinterface.py line 258

The problem is, as written, this test is not testing anything. You can leave it here but if session termination is not currently testable, we should maybe open an issue in tinker-engine to make it so.

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 3, 2020, 14:03

Commented on tests/test_parinterface.py line 258

Sure, although wouldn't the issue be in sail-on-client and maybe on sail-on too in case the server needs to provide meaningful response?

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @waxlamp on Sep 3, 2020, 14:07

Commented on tests/test_parinterface.py line 258

You're right, wherever the terminate_session() method is defined.

Just to be clear, I approved this particular MR already so you can merge it when you're ready.

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 3, 2020, 14:08

Commented on tests/test_parinterface.py line 258

Sounds good #23 would address the issue

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 3, 2020, 14:08

Commented on tests/test_parinterface.py line 258

Waiting on Adam's review too

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @awk11 on Sep 3, 2020, 15:34

Commented on tests/test_parinterface.py line 258

Sorry for the delay, today's been busy. As for this question though, terminate session simply returns either the string "OK" or passes along any error on the api end. So you can either have the client side of it pass that along if you want to be able to assert against something, or can just leave it as is and the test is to ensure that no errors are thrown. This is the same in post_results

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @awk11 on Sep 3, 2020, 15:38

Everything looks good to me

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @awk11 on Sep 3, 2020, 15:38

resolved all threads

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 3, 2020, 15:40

Commented on tests/test_parinterface.py line 258

Thanks for the info, I can use "OK" and update the tests in two methods along with two methods in #23

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 3, 2020, 15:41

mentioned in commit dc397af

@as6520
Copy link
Contributor Author

as6520 commented Mar 5, 2021

In GitLab by @as6520 on Sep 3, 2020, 15:41

merged

@as6520 as6520 closed this as completed Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant