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.

Sign up for GitHub

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

Jump to bottom

Add CI configuration and tests, perform simple fixes #17

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

Add CI configuration and tests, perform simple fixes #17

wants to merge 5 commits into from

Conversation

honzajavorek
Copy link
Contributor

@honzajavorek honzajavorek commented Apr 15, 2020 •
edited
Loading

Implements and closes #13. Doesn't make the tests pass, because I was unable to debug the following problems:

/Users/honza/Projects/emojicodes/docs/source/index.rst:5: WARNING: Inline emphasis start-string without end-string.
/Users/honza/Projects/emojicodes/docs/source/index.rst:5: WARNING: Inline emphasis start-string without end-string.
/Users/honza/Projects/emojicodes/docs/source/index.rst:5: WARNING: Inline emphasis start-string without end-string.

If those are fixed, I believe the tests will pass. Under honzajavorek#1 it should be visible how it works.

The Windows tests will probably fail as well even when the other will pass, because I'd say make won't be installed out of the box, but that can be later fixed simply by choco install make I think. Let's see. It'd focus on making it pass on Linux/macOS now. Also, perhaps it doesn't make sense to test the docs on multiple OS'es and it's okay to test it just on Linux. We'd test on multiple OS'es only if we have real tests for the code. But so far I'm keeping what I have as a demonstration of what's possible if we make it pass, and we can expand on that / restructure it later.

@Peque Peque self-requested a review April 17, 2020 17:08
Peque
Peque requested changes Apr 17, 2020
View reviewed changes
Copy link
Member

@Peque Peque left a comment

Choose a reason for hiding this comment

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

@honzajavorek Thanks for having a look at this! 😊

I have nothing against GitHub Actions (or any other CI service integration). I would, however, prefer to make it easy for developers to be able to run the tests locally. Also, that eases any possible CI service migration in the future.

I would use Tox for this. See for example osbrain's Tox configuration and how Travis runs just Tox.

Could you try to put the CI checks in a tox.ini file and then run the tests with Tox from GitHub Actions? 😇

@honzajavorek
Copy link
Contributor Author

Oh, tox supports running arbitrary commands? I thought it's just for tests of code (e.g. pytest), that's why I skipped it for this stage (also, the wording used in #13 made me think introducing tox could be a next step).

@Peque
Copy link
Member

Peque commented Apr 17, 2020

@honzajavorek Yeah, you are right interpreting #13. However, since it seems you are on a streak I thought maybe we could take advantage and hit two targets with one shot! 😜

@honzajavorek
Copy link
Contributor Author

I'll check out tox, but no promises about timing :)

@Peque Peque added the enhancement New feature or request label May 4, 2020
@Peque Peque self-assigned this May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Peque Peque Peque requested changes

Assignees

@Peque Peque

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test documentation
2 participants
@honzajavorek @Peque