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

chore: use start-stack command for e2e tests, replace Makefile by poetry-exec-plugin (DEV-1597) #279

Merged
merged 19 commits into from
Jan 16, 2023

Conversation

jnussbaum
Copy link
Collaborator

@jnussbaum jnussbaum commented Jan 10, 2023

resolves DEV-1597

  • make docs/index.md more suitable for PyPI landing page
  • include "anything" onto+data in start-stack command, so that this command can be used for the e2e tests
  • replace Makefile by poetry-exec-plugin
  • first time using Semantic Line Breaks (https://sembr.org/)
  • remove daily tests, because now, the e2e tests don't run any more with the daily-changing main branch of DSP-API, but always with the same Docker images
  • tidy up all YML GitHub Actions files, modify/remove some of the names

@jnussbaum jnussbaum self-assigned this Jan 10, 2023
Copy link

@irinaschubert irinaschubert left a comment

Choose a reason for hiding this comment

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

LGTM

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
src/dsp_tools/utils/stack_handling.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

looks good over all, just some minor suggestions.
To fix the failing tests, I think you can just use the full admin (and maybe permissions) data turtle files, instead of the minimal ones.

.github/workflows/release-please.yml Show resolved Hide resolved
.github/workflows/tests-on-push.yml Show resolved Hide resolved
.github/workflows/tests-on-push.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
Comment on lines 21 to 23
poetry run dsp-tools start-stack --no-prune
-poetry run pytest test/ # ignore errors, continue anyway with stop-stack (see https://www.gnu.org/software/make/manual/make.html#Errors)
poetry run dsp-tools stop-stack
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could also look into the poetry exec plugin to automate poetry workflows, which might spare you even more of the makefile

Copy link
Collaborator

Choose a reason for hiding this comment

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

(for a discussion, see here python-poetry/poetry#241)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, it would be cool to get rid of the Makefile, esp. because there are only 2 commands left in it, each a 1-liner. But I'm a bit reluctant using a plugin which is still pre-1.0... What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be worth trying, even if it's only to see how well it works. In any case, it would be easy enough to revert back to having the make file (and if it's only two one-liners, you could just as well put them into the readme and execute them manually).
Speaking of those two commands: in the make clean command, are most of them still needed? I would guess that both mkdocs (for the docs) and poetry (for building) take care of removing old files pretty well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, done :-)
There is only one exec-script remaining, and that one is only used by me. This allowed me to keep the documentation of the poetry-exec-plugin minimal.

src/dsp_tools/utils/stack_handling.py Outdated Show resolved Hide resolved
src/dsp_tools/utils/stack_handling.py Outdated Show resolved Hide resolved
src/dsp_tools/utils/stack_handling.py Outdated Show resolved Hide resolved
Please note that testing requires launching the complete DSP API stack which is based on docker images.
Therefore, we recommend installing the [docker desktop client](https://www.docker.com/products).
To run the complete test suite:
The tests of this repository
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I use Semantic Line Breaks (https://sembr.org/) for the first time. It sounds convincing, but I'm curious to hear your feedback.

One of the big advantages, which is not even described in the specification: we will get cleaner git diffs, because changing a single word in the middle of a paragraph won't force us any more to change all line breaks of that paragraph.

@jnussbaum jnussbaum changed the title chore: use start-stack command for e2e tests (DEV-1597) chore: use start-stack command for e2e tests, replace Makefile by poetry-exec-plugin (DEV-1597) Jan 13, 2023
@jnussbaum jnussbaum changed the title chore: use start-stack command for e2e tests, replace Makefile by poetry-exec-plugin (DEV-1597) chore: use start-stack command for e2e tests, replace Makefile by poetry-exec-plugin, make docs/index.md more PyPI-friendly (DEV-1597) Jan 13, 2023
@jnussbaum jnussbaum changed the title chore: use start-stack command for e2e tests, replace Makefile by poetry-exec-plugin, make docs/index.md more PyPI-friendly (DEV-1597) chore: use start-stack command for e2e tests, replace Makefile by poetry-exec-plugin (DEV-1597) Jan 13, 2023
@jnussbaum jnussbaum merged commit 6b85d15 into main Jan 16, 2023
@jnussbaum jnussbaum deleted the wip/dev-1597-use-start-stack-command-for-e2e-tests branch January 16, 2023 08:27
@daschbot daschbot mentioned this pull request Jan 16, 2023
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.

None yet

3 participants