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

Refactor tests and add more tests #85

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

auouymous
Copy link
Contributor

No description provided.

Copy link
Owner

@donniebreve donniebreve left a comment

Choose a reason for hiding this comment

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

Please provide reasoning for this refactoring.

@auouymous
Copy link
Contributor Author

It avoids mistakes like #76.

It tracks the total number of tests and number of failed tests, returning a non-zero status code upon failure. The tests could now be run in the test.yml workflow.

It condenses the duplicate code of every test making them easier to write and read. The named keys also aid in readability, replacing the comment of many tests.

The second commit adds tests for features that were missing them. There are also sample tests at the bottom which could be implemented now that it is easier to write them, and the new syntax closely resembles that of the samples.

@donniebreve
Copy link
Owner

donniebreve commented Oct 1, 2024

It avoids mistakes like #76.

I don't think it does. Mistakes will happen, regardless of how crafty your code is.

It tracks the total number of tests and number of failed tests, returning a non-zero status code upon failure. The tests could now be run in the test.yml workflow.

This is a useful addition which wouldn't require adding string representations of the key codes. The tests do run in test.yml.

It condenses the duplicate code of every test making them easier to write and read. The named keys also aid in readability, replacing the comment of many tests.

This I can agree with, although I am against the implementation. I would prefer using the KEY_ identifiers instead of arbitrary names.

The second commit adds tests for features that were missing them. There are also sample tests at the bottom which could be implemented now that it is easier to write them, and the new syntax closely resembles that of the samples.

Additions like TYPE("hyper down, seq tap, hyper up", EXPECT, "seq1 down, seq2 down, seq1 up, seq2 up"); are really no more readable than the previous implementation. The expected output requires understanding more than the words used here.

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.

2 participants