stefankroes / ancestry

Organise ActiveRecord model into a tree structure
MIT License
3.72k stars 458 forks source link

Tests more closely follow suggested configurations #624

Closed kbrock closed 1 year ago

kbrock commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

I hadn't noticed that https://github.com/stefankroes/ancestry/pull/597 dropped all the materialized_path testing.

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

kbrock commented 1 year ago

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 https://github.com/stefankroes/ancestry/pull/597.

[...it is still there...], I just renamed STRATEGY to FORMAT in https://github.com/stefankroes/ancestry/pull/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 https://github.com/stefankroes/ancestry/pull/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.

kbrock commented 1 year ago

update:

@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?

kbrock commented 1 year ago

update:

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