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

Techdebt: allow test & lint to run for specific service #6024

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hans-d
Copy link
Contributor

@hans-d hans-d commented Mar 7, 2023

make test SERVICE_NAME=s3

Makefile Outdated
@echo "Running MyPy..."
mypy --install-types --non-interactive
mypy --install-types --non-interactive moto/${SERVICE_NAME}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not all services support typing at the moment - the ones that do are listed in setup.cfg. This is a very quick check, it only takes a few seconds, so I would suggest to just leave this as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it a bit, but we can go this route as well.

hans-d added 4 commits March 7, 2023 15:39
- mypy was configred via `files` which fails when specifying a target dir. Now turned into an exclude in the config, and adding explicit files to the Makefile when a service is selected.
- added SKIP_MYPY to be able to skip mypy check (as long as the mypy chnage is not complete)
- using conftest.py to give all the tests default markers
- the include/exclude from the makefile is now in the conftest.py marking as well
- for services, also picking up test that reference that service in its name (specific pattern)
@hans-d hans-d requested a review from bblommers March 7, 2023 22:15
parallel_server_only: can be run in parallel (in server mode only)
skip_server: should not be run in server mode
# services
acm: acm service
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get this feature without listing all services? This would be a pain to maintain, considering the list changes quite 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.

pytest will complain if these marks are not specified. Perhaps I can add something to ease this maintenance (or at least check all are there)

@@ -229,7 +352,14 @@ disable = W,C,R,E
enable = anomalous-backslash-in-string, arguments-renamed, dangerous-default-value, deprecated-module, function-redefined, import-self, redefined-builtin, redefined-outer-name, reimported, pointless-statement, super-with-arguments, unused-argument, unused-import, unused-variable, useless-else-on-loop, wildcard-import

[mypy]
files= moto/a*,moto/b*,moto/c*,moto/d*,moto/e*,moto/f*,moto/moto_api,moto/neptune
files= moto/a*,moto/b*,moto/c*,moto/d*,moto/e*,moto/moto_api,moto/neptune
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes often as well, we're continuously adding typing support for new services, so I'd rather to just have a simple list. Having to understand and get the regex right isn't worth the effort

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, will revert this bit

@bpandola
Copy link
Collaborator

bpandola commented Mar 8, 2023

While I appreciate the spirit of this PR, I think it's too big of a change and too much of a future maintenance hassle for the benefits. Locally testing just the service you've worked on is already quite simple, e.g. pytest -sv ./tests/test_redshift. (A simply note in the development docs could make this more clear.)

make lint takes a lot longer than it used to, but it's still not that onerous, considering you really only need to run it once at the end of your local development.

Happy to consider better arguments for the need here, but I'm not currently in support of this.

@hans-d
Copy link
Contributor Author

hans-d commented Mar 8, 2023

The tagging of resources comes into play, eg when not just wanting to run all cloudformation tests in the tests_cloudformation folder, but also related test in other folders (or for sns, lambda, ....)

A similar list for the services already needs to be maintained for the dependencies. We can piggyback on that re checking for completeness / use the an additional check that also validates that this list dependency list is complete (not re the dependencies added, but at least having an entry present).

@bblommers
Copy link
Collaborator

@bpandola The first commit was simpler (02b516c). A change like that would be a nice QOL-improvement IMO, to run lint/test on a specific folder.

Agree that having another explicit list of services is too much of a hassle.

@hans-d
Copy link
Contributor Author

hans-d commented Mar 8, 2023

shall I revert to the first one?

@bblommers
Copy link
Collaborator

Yes, let's, @hans-d, I think that's better.

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