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

Typos adjusts #699

Merged
merged 2 commits into from
Jun 2, 2024
Merged

Typos adjusts #699

merged 2 commits into from
Jun 2, 2024

Conversation

djpremier
Copy link
Contributor

@djpremier djpremier commented May 16, 2024

Hi,

These are some small errors that I found while updating my application. If you need it to be in different PRs, I can do that too.

@ddnexus one question I got is how you are running the command to regenerate the Rematch files, looking in its documentation I found the command rake test TESTOPTS=--rematch-rebuild, however with this all the files were changed basically in the order in which tests are saved, would this be expected? Or is there some command that forces a certain ordering?

Complete command I used (it can even be mentioned in the contribution doc for those who don't "know" how to run it):
docker run -it --rm --name pagy-script -v "$PWD":/usr/src/pagy -w /usr/src/pagy ruby:latest /bin/bash -c "bundle install && rake test TESTOPTS=--rematch-rebuild"

Example of result with divergence:
image

@ddnexus
Copy link
Owner

ddnexus commented May 17, 2024

Thank you @djpremier. Great PR! Code, test and docs fixes! You even removed the trailing spaces at EOL.

(RubyMine has a built-in option for that but it ignores .md files, and I miss a lot of them) 🤷

PR split

  • Unless required by code changes, I would prefer to avoid gem updates, that are usually done with npm module updates in their dedicated commits. I believe this PR can avoid it.

  • ATM it's better to put the docs fixes in their own PR, so they can be merged directly into master and published right away.

  • Changes in the gem dir (and related test files) must be merged and tested in dev so it's easier to have that in a different PR. Notice however that the PR itself should be based on master: we will change the base branch before merging.

Rematch

The --rematch-rebuild option rebuilds ALL the rematch files which tests are run, which basically means that it deletes each previous *.rematch file and stores whatever are the resulting values from each test file run.

If you run all files with the rebuild option, you rebuild also the files that didn't fail, thus some value may get reordered inside the new file, despite the actual values are the same. Besides you might permanently hide failures by storing the failed value as the expected value, if you didn't get a chance to check it first.

A safer way to update the rematch files is checking each failure and reconciling one file at the time, either by running the single file with the option, or by manually deleting the .rematch file before running the single test file again.

@djpremier
Copy link
Contributor Author

Thanks for the feedback @ddnexus 😊.

As for removing spaces, it is automatic when I save a file hehe. As I use VS Code, I adjusted 2 settings in it to avoid conflicts and remove these unwanted "things", they are:
image

Rematch

Yes, I only left what related to my changes, my question was whether there was anything I should do to maintain order. I will analyze your library when possible, maybe there is something we can adjust so that the result is always the same for the same content.

@djpremier
Copy link
Contributor Author

As for gems, it's a habit of mine to try to keep projects as up to date as possible, to avoid having too many changes to keep track of later, but if you already have your way you can maintain it without any problems. I created the new PR for the code fixes and only uploaded the update necessary for the CI to work.

#700

@ddnexus
Copy link
Owner

ddnexus commented May 17, 2024

maybe there is something we can adjust so that the result is always the same for the same content

Minitest doesn't run the tests in order, because in the author opinion tests should be atomic and shouldn't need any particular sorting in order to pass. I have to agree.

That means that instead of storing/reading the values from the store file in sync to their executions, we would need to keep the values in some variable. When the test file is done running, sort the values according to the location of the rematch call and write the ordered store file.

We would also have to consider what to do when you just reorder the tests. Should it fail or not? Should we rewrite the store file regardless (for consistency)? Automatically? Writing while we are supposed to read? Should we just warn about it? It looks like we would add quite a lot of complexity.

All that is possible, but I am not sure the benefit of having the ordered store file justifies the overhead and the added maintenance burden, but if you find a better way to do it... you will be my hero! 🙂

@ddnexus ddnexus merged commit 0ee978d into ddnexus:master Jun 2, 2024
4 checks passed
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

2 participants