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

docs: improve developer doc for testing #48

Merged
merged 10 commits into from
Oct 12, 2023
Merged

Conversation

indecisivedragon
Copy link
Contributor

Issue

closes #https://github.com/crashappsec/chalk-internal/issues/835

Description

This updates the README.md in tests to be more informative and useful.

tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated
make tests args="[TESTFILE]::[TESTNAME][test-case]"
```

ex: `make tests args="test_elf.py::test_virtual_valid[copy_files0]"`
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move example into code block above. it will render better vs being inlined


### Container Shell

You can also drop into a shell in the ` tests` container via `docker compose run --entrypoint=sh tests`, and from there run `pytest` manually.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add make tests/sh or something to fasciliate that? I think you need to do that pretty often

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do do it pretty often, a make command for this would be great! Let's do it in another pr and update the doc there, I'd like to keep this one just docs without code changes.

tests/README.md Outdated

### Datafile Location

All new test files should added to the `tests/` directory, and any test data should be added to the `tests/data` directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
All new test files should added to the `tests/` directory, and any test data should be added to the `tests/data` directory.
All new test files should be added to the `tests/` directory, and any test data should be added to the `tests/data` directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does vale catch grammar issues also? 🙀

Copy link
Contributor

Choose a reason for hiding this comment

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

some basics I think if you enable appropriate styles (they have defaults for microsoft, google and redhat - https://vale.sh/generator/). this was just me but Im not English expert so I have no idea if its actually correct. my logic if it sounds right, its prolly right but what "right" actually is 🤷

tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated
- New test files should be added to the `tests/` directory as
`test_filename.py`.
- Individual tests within the test file should be named `test_testname`.
More information about fixtures can be found [here](https://docs.pytest.org/en/6.2.x/fixture.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

lets use consistent version of pytest for all pytest links. above there are links to 7.x but here its 6.x

tests/README.md Outdated Show resolved Hide resolved

## Setup

Before running tests, ensure that `docker` and `docker compose` are installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we list dependencies and give an option for how to run this stuff NOT in docker? Obviously some Linux-specifiic stuff would break, but:

a) The process does slow things down a lt
b) We probably should run tests suites natively on a Mac anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added instructions with several caveats.

viega
viega previously approved these changes Oct 11, 2023
Copy link
Contributor

@viega viega left a comment

Choose a reason for hiding this comment

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

Two small additions then it's fine to merge

Tests will always rebuild the binary if it detects a code change in the underlying nim code. However, you will need to build `chalk` manually when switching between `release` and `debug` builds:

1. By default, the testing script will build a `release` binary for chalk, so if you want to test against the `debug` build instead you must build it yourself.
2. If you have been working with a `debug` build so far, but want to switch to `release`, you will need to rebuild manually as the test script will not rebuild if there have not been any code changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I got the manual instructions running on main, but the tests don't really work there:

========================== 50 failed, 32 passed, 17 skipped, 2 errors in 100.05s (0:01:40)

Some, maybe all of it, is clearly due to an assumption of ELF. I think that's a bit of an issue. But at the very least it needs to be clear in the docs that they are ELF only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have added a warning at the top

tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated
Comment on lines 127 to 128
2. Start poetry shell with `poetry shell`
3. Run tests with `pytest` (flags and arguments as in the previous section)
Copy link
Contributor

Choose a reason for hiding this comment

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

poetry shell has some implications which might break for some people. in docs it might be simpler to do poetry run pytest .... which internally will activate appropriate venv and then run pytest in that env without explicitly activating any of the shell bits (bash/zsh/fish/etc)

miki725
miki725 previously approved these changes Oct 12, 2023
@indecisivedragon indecisivedragon merged commit 8664d9b into main Oct 12, 2023
2 checks passed
@indecisivedragon indecisivedragon deleted the ll/testing_dev_doc branch October 12, 2023 14:38
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.

3 participants