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

Tests more closely follow suggested configurations #624

Merged
merged 2 commits into from
Mar 11, 2023

Conversation

kbrock
Copy link
Collaborator

@kbrock kbrock commented Mar 10, 2023

Test both materialized_path

I hadn't noticed that #597 dropped all the materialized_path testing.
Since a majority of the users are using materialized_path, we need to make sure this is working front and foremost

Setting the column to nulls: true only for materialized_path

Separate update mode

Before

We were testing postgres only with :sql update mode.

After

This behavior has not changed. But now it is more explicit.

It seems fine to only test postgres with :sql update mode since
both mysql and sqlite paths are testing the traditional :ruby update mode.

Test mysql with binary columns

Running mysql with both ascii and binary columns since we support both.

Support matrix

Rails 5.2 and 6.0 (and the corresponding ruby 2.5 to 2.7) are basically EOL. Lets keep them until they hold us back.

Due to the way I've setup the matrix, I need to choose a single rails version to test with 7.0. I chose 2.2 since that is rail's suggested version.

This leaves us not testing ruby 2.3. If anyone knows how to add both 2.2 and 2.3, please share.

@kbrock
Copy link
Collaborator Author

kbrock commented Mar 11, 2023

@kshnurov could you please share the use case where this is running the same tests twice?

Are you thinking we only need to run materialized_path for one or two use cases rather than for all test combinations?

@kbrock
Copy link
Collaborator Author

kbrock commented Mar 11, 2023

@kshnurov I am holding off merging this because you expressed dissatisfaction with this PR. But I don't know what your issue is.

These changes are useful in helping me diagnose problems with counter caching and depth caching. (an issue that you asked me to look into)

Please share what your issue is and I will do my best to resolve it.

@kshnurov
Copy link
Contributor

I hadn't noticed that #597 dropped all the materialized_path testing.

Because it didn't drop anything, it's still there. See #622

@kbrock
Copy link
Collaborator Author

kbrock commented Mar 11, 2023

Paraphrasing @kshnurov comments and reformatted a little from another thread. Best to have a conversation in one place:

I am adding back the backwards compatible tests that you dropped in #597.

[...it is still there...], I just renamed STRATEGY to FORMAT in #597.
It is testing both strategies, without FORMAT it'll be the default materialized_path.

bundle exec rake
FORMAT=materialized_path2 bundle exec rake

You've added format to matrix in #624, which is really good, but you didn't remove the first bundle exec rake, and now all tests are run twice. [... there are ...] two consecutive bundle exec rake here.

suggestion:

- bundle exec rake
  FORMAT=#{{ matrix.format }} bundle exec rake

Thank you for finding that.

This is not really a value we should be exposing

The test globally set the format, so they should know what format is being used
without asking
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 11, 2023

update:

  • removed duplicate rake
  • fixed testing so all versions of ruby are tested
  • testing postgres with both update strategies.

@kshnurov I was thinking about moving the format back into the single test run. This time into their own sections. It will be well documented like we do with databases.

Why:

The container setup overshadows the time it takes to run the tests. Things like obtaining a container adds additional hidden overhead. So while each test run is maybe 30 seconds faster, there are double the number of runs - so the overhead is much more pronounced.

@kshnurov It sounded like you liked the matrix addition, so I wanted to ask if you had a strong opinion if I moved those back into the individual runs?

- properly configure null columns with materialized_path2
- support and test binary columns (for mysql)
- make update_strategy=:sql for postgres explicit rather than auto selected
- display options to console (probably temporary)
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 11, 2023

update:

  • fixed heading for mysql test

ok. This seems to run very quickly right now.
If it poses a problem in the future, will possibly merge them together in the future

@kbrock kbrock merged commit 7621f09 into stefankroes:master Mar 11, 2023
@kbrock kbrock deleted the github_matrix_tests branch March 11, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants