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

Skeleton dev #278

Merged
merged 31 commits into from
Dec 12, 2024
Merged

Skeleton dev #278

merged 31 commits into from
Dec 12, 2024

Conversation

kebwi
Copy link
Collaborator

@kebwi kebwi commented Dec 9, 2024

This PR introduces a small piece of code to alleviate the recent notice_text issue, and then adds minimal boiler-plating for skeleton tests, successfully running and passing. The next step will be to add more substantial tests now that the basic testing infrastructure for the new client is in place.

kebwi and others added 22 commits November 21, 2024 12:05
* draft pyproject.toml

* refactor versioning logic

* try redo dev workflow

* add lockfile

* try sync

* specify test dev

* lock

* remove 3.7

* add ipython

* try no ipython again

* add ipython back in

* add new lock

* test on 3.8 again

* skip locking

* fix venv command

* try uv run

* try oldest supported numpy

* docs

* rename

* fix up actions

* wip actions

* action fixes

* add pyproject.toml to bumpversion

* fix action

* change message name
* adding leaves many

* add test

* fix test

* fix test again

* move to seperate method

* cleaning up doc string

* fixing dictionary return

* fix formatting

* fixing test
* add type hinting to jsonservice

* add get_state test

* omit linting of string type hint
* fixing minimal covering nodes

* adding version constraint

* adding version constraint

* add version 2 range constraint

* adding test for new feature

* fixing whitespace

* ignoring notebooks in formatting

* fixing formatting

* updating uv.lock

* including tests and docs in linting
@kebwi kebwi requested a review from fcollman December 9, 2024 23:27
@fcollman
Copy link
Collaborator

should add a test for this client side functionality of notice text

@kebwi
Copy link
Collaborator Author

kebwi commented Dec 11, 2024

While I take your point about the additional test, the point of the PR was just to get minimal testing functionality in place, a scaffold from which I would extend the tests in subsequent work. The fact that the notice_text fix was in this PR at all was merely an admittedly imprecise comingling of two parallel tasks: adding a testing framework (for subsequent extension) and fixing the notice_text bug. I didn't mean to leave the latter in an implied-unfinished state.

All that said, I don't understand the test you added. Since it lacks any assertion statements, the only way it could fail is by some sort of raised Exception. It doesn't actually test the correctness of the action it purports to perform. Anyway, I didn't mean to leave it out as an oversight. I just didn't consider the tests to be remotely complete anyway. I was just getting a minimal testing framework in place for future expansion.

@kebwi
Copy link
Collaborator Author

kebwi commented Dec 11, 2024

And more annoyingly, as usual, seemingly trivial code changes fail without lint/ruff changes, as you can see from the results above. I'll fix it.

@fcollman
Copy link
Collaborator

the assertion is a good point, the test did test that the endpoint that was hit was hit with a notice_text="" json body when i passed in a notice_text="None" string, if it didn't do that mapping then the mocker wouldn't have found a match and the test would have failed, but it is good to have a check on the response.

@fcollman fcollman merged commit 9ea21a5 into master Dec 12, 2024
13 checks passed
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.

4 participants