Adapted skills:check and skills:install and added reusable tests - #966
jana-selva wants to merge 7 commits into
Conversation
GitHub Actions
Waiting
GitHub Actions
Waiting
jana-selva
changed the title
jana-selva
changed the title
| ] | ||
|
|
||
|
|
||
| def test_tasks_exports_skill_tasks(): |
There was a problem hiding this comment.
This test I would remove.
I think if we wanted something like this it'd be better to have an integration test running nox -l. That way we could check that the exposed nox sessions are the same against a hard-coded list.
There was a problem hiding this comment.
Ah, I think you tried to resolve this by adding test_check_runs_all_project_checks , but I would have you remove test_check_runs_all_project_checks and really check nox -l via a test similar to how test_skills_install_and_check is set up.
| ] | ||
|
|
||
|
|
||
| def _validate_eval_cases(eval_cases: object, skill_name: str) -> list[str]: |
There was a problem hiding this comment.
Please break this up into smaller test functions.
Possible ideas are:
-
Create a validator class which has these methods (mostly private) that uses them in a "main" function.
-
As we're in a testing suite, it might be better to break these up into test methods inside a test class. You can then still iterate over the cases with 1 declaration of
@pytest.mark.parametrize("skill_name", _skills_with_eval_cases())over the class itself. [While I believe you don't need it, if you did need the tests to run in a certain order, you can either group critical ordered ones together in one method or add https://pypi.org/project/pytest-order/#description as a dev dependency and mark the tests.]
There was a problem hiding this comment.
Fixed, please re-review
There was a problem hiding this comment.
Ah, that's not exactly what I meant.
Instead of appending errors, and then checking it, I meant that _validate_eval_cases should be removed and the components it tests for are done individually, like in a structure similar to TestEvalCaseValidation. This would be a much clearer design and easier to debug if an issue arose in the future.
|
❌ The last analysis has failed. |
GitHub Actions
Waiting
GitHub Actions
Waiting
|
| .. _developer_agent_skills: | ||
|
|
||
| Testing Agent Skills | ||
| ==================== |
There was a problem hiding this comment.
💚 Thanks, this is fantastic and exceeds what I was hoping for.
| docs_list_links(nox_session) | ||
|
|
||
|
|
||
| def test_build_docs_runs_sphinx(nox_session, config): |
There was a problem hiding this comment.
Can you give me more context as to why you added these tests? It's not immediately clear to me in the scope of this issue. If it's just to raise the test coverage, I'd rather this be done in a detected manner.
The Sphinx documentation has many Sonar issues, and it'd be best to resolve those in coordination with the coverage.
| build.assert_called_once_with(nox_session, config) | ||
|
|
||
|
|
||
| def test_build_docs_command(config): |
There was a problem hiding this comment.
These kinds of tests are require lots of maintenance, if the code is changed.
| docs_list_links(nox_session) | ||
|
|
||
|
|
||
| def test_build_docs_runs_sphinx(nox_session, config): |
There was a problem hiding this comment.
test_build_docs_runs_sphinx seems not needed. It only verifies that build_docs() delegates to _build_docs(), while the actual command behavior is already covered by test_build_docs_command. This couples the test to the current internal helper structure rather than testing user-visible behavior. If the documentation build implementation changed, this test would need to change even if the public behavior remained correct.
There was a problem hiding this comment.
Tests like this can also lead to the false impression that there's coverage, as it executes many of the lines, but we're not really testing what we expect it to do.
| build.assert_called_once_with(nox_session, config) | ||
|
|
||
|
|
||
| def test_build_multiversion_docs_runs_sphinx(nox_session, config): |
There was a problem hiding this comment.
Same reason on this one:
https://github.com/exasol/python-toolbox/pull/966/changes#r4144249890
| session.run.assert_any_call("touch", f"{DOCS_OUTPUT_DIR}/.nojekyll") | ||
|
|
||
|
|
||
| def test_open_docs_reports_missing_output(nox_session, config): |
There was a problem hiding this comment.
test_open_docs_reports_missing_output only verifies that _SessionQuit is raised, not that the expected error is reported. It would be more useful to assert the message passed to session.error(), or remove the test if exception propagation itself is not part of the contract. As written, it mainly provides branch coverage
This branch was successfully deployed
2 participants
Fixes #963
Checklist
Note: If any of the items in the checklist are not relevant to your PR, just check the box.
For any Pull Request
Is the following correct:
When Changes Were Made
Did you:
When Preparing a Release
Have you: