Skip to content

Split and prioritize integration tests execution#3189

Closed
dottorblaster wants to merge 3 commits intoapache:mainfrom
dottorblaster:split-prioritize-integration-tests-exec
Closed

Split and prioritize integration tests execution#3189
dottorblaster wants to merge 3 commits intoapache:mainfrom
dottorblaster:split-prioritize-integration-tests-exec

Conversation

@dottorblaster
Copy link
Member

Overview

This PR is a follow-up to #3110 and a should allows us to close #1885.

We want to adopt a fail-fast approach so we split tests in several categories and we're running them from the fastest one to the slowest.

Testing recommendations

If make elixir is ok, then you'll probably be ok. I didn't know if there would be a better way of handling these modifications in the Makefile.

Related Issues or Pull Requests

#3110, #1885

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

@dottorblaster dottorblaster requested a review from iilyak October 4, 2020 16:43
@dottorblaster dottorblaster self-assigned this Oct 4, 2020
@dottorblaster
Copy link
Member Author

@iilyak I need your opinion here 😄 actually I discovered that it's not possible to run --only and exclude stuff, mainly because inclusion takes precedence over any exclusion, so even if we exclude some pending tests they are included again as part of a (example given) kind: :single_node test suite.

The only solution I found was adding a @tag kind: :pending to those modifying indeed the kind label of the test. This way pending tests were properly excluded.

Do you think this is feasible? Pending tests are like less of ten, so I'd follow this approach and do some PRs following this trying to resolve those pending cases, just to get rid of this stuff.

@iilyak
Copy link
Contributor

iilyak commented Oct 5, 2020

The approach is appropriate. However there is a little problem. Some tests would run twice. For example, cluster_with_quorum_test has two tags: with_quorum_test and cluster. Therefore we run it via elixir-cluster and elixir-cluster-with-quorum.

I think we need to:

  1. remove elixir-cluster-with-quorum (since it is covered by elixir-cluster).
  2. replace elixir-cluster-without-quorum with elixir-degraded-cluster (which would use --only)

@dottorblaster
Copy link
Member Author

Absolutely, I noticed a couple hours ago, my bad. Tomorrow I'll apply fixes to the Makefile and commit the different kindtags, thanks a lot 😄

@dottorblaster dottorblaster force-pushed the split-prioritize-integration-tests-exec branch from 71a9c04 to c4a122a Compare October 6, 2020 13:30
@dottorblaster dottorblaster changed the base branch from master to main October 7, 2020 08:34
@dottorblaster dottorblaster force-pushed the split-prioritize-integration-tests-exec branch 4 times, most recently from f7b8405 to f52037c Compare October 15, 2020 20:27
@dottorblaster dottorblaster force-pushed the split-prioritize-integration-tests-exec branch 3 times, most recently from 1c01924 to aad64ed Compare October 21, 2020 20:37
Copy link
Contributor

@jjrodrig jjrodrig left a comment

Choose a reason for hiding this comment

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

I think that with this change we are losing the quorum related tests.

elixir-degraded-cluster: export MIX_ENV=integration
elixir-degraded-cluster: export COUCHDB_TEST_ADMIN_PARTY_OVERRIDE=1
elixir-degraded-cluster: devclean
@dev/run "$(TEST_OPTS)" -a adm:pass -n 1 \
Copy link
Contributor

Choose a reason for hiding this comment

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

@dottorblaster this configuration is the same that the elixir-single-node option. To define a degraded cluster it is required to startup multiple nodes and then stop some of them. I included this option in the run script for the without_quorum testing.
I think that we should keep these options to launch a degraded cluster

 @dev/run -n 3  --degrade-cluster 2  ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Bloody hell you're right, sorry man

@dev/run "$(TEST_OPTS)" \
-a adm:pass \
-n 1 \
.PHONY: elixir-cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this is not enough for running the with_quorum tests. These tests require a degraded cluster with quorum as some of the test are checking the quorum override behaviour of some operations. We need a degraded cluster that keeps the quorum for this.

 @dev/run -n 3  --degrade-cluster 1  ...

Copy link
Member Author

Choose a reason for hiding this comment

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

As above :) Sorry!

@dottorblaster
Copy link
Member Author

@jjrodrig theoretically they should be inside the cluster and degraded_cluster categories, I'm having a hard time making the test suite run on Jenkins for some reason though

@dottorblaster dottorblaster force-pushed the split-prioritize-integration-tests-exec branch from 03369ea to fec646f Compare November 6, 2020 16:34
@dottorblaster
Copy link
Member Author

@iilyak are these efforts still valid? I saw you merged a PR (#3286) to overcome an issue I was stuck in front of 👍

You think this approach is still valid?

@iilyak
Copy link
Contributor

iilyak commented Dec 18, 2020

@iilyak are these efforts still valid? I saw you merged a PR (#3286) to overcome an issue I was stuck in front of 👍

I didn't know that inability to skip tests is what motivated you for this work. I implemented #3286 as an additional target to call instead of make elixir for people who run customized versions of CouchDB.

The idea was that CouchDB CI would use your work. We possibly use different tags for things which are not implemented yet for FDB based CouchDB. Maybe 3.x tag for all elixir tests and 4.x for tests which already working in main branch. This should cover all possible combinations:

3x 4x description
+ New APIs (new pagination API from #2870 for example)
+ APIs which are not yet implemented in main or removed
+ + APIs which already work for main and master

However these new tags might interfere with kind. Which probably means we probably need to comment out 3x tests which are testing not implemented yet features.

Again the #3286 switches to make elixir-suite as a temporary solution. Nick noticed that the coverage via #3286 is greater than the one via make elixir tests=test/elixir/test/basics_test.exs,test/elixir/test/replication_test.exs,test/elixir/test/map_test.exs,test/elixir/test/all_docs_test.exs,test/elixir/test/bulk_docs_test.exs so he proposed to use new way for now.

The deficiency of #3286 is the need to maintain test/elixir/test/config/suite.elixir. It is ok for people who run customized versions, but IMO it is not good enough as a primary mechanism. Ideally the ability to skip tests via blacklist should be supported by testing framework. I considered opening PR for mix. However I realized that CouchDB wouldn't benefit from the feature because it is going to be a long time before we migrate to version of Elixir which would have my patch.

@dottorblaster
Copy link
Member Author

dottorblaster commented Dec 20, 2020

Hm. I'll think about that, honestly I was struggling with this because the --only flag has some quirks, I'll analyze this again and report.

Ideally the ability to skip tests via blacklist should be supported by testing framework

Mix is not the right place for this discussion to happen, maybe you'll want to speak to the folks maintaining ExUnit :-D nonetheless I think they are pretty much the same team, you're right.

Anyway your fix makes CouchDB have a much wider coverage because you include test modules in a very specific way, while --only as far as I can remember cannot be combined with exclusions or something like that. To be honest that was the reason I dropped things on this PR, I plan to take on this again as your work was very inspiring for a new solution. Still, I don't think we will be allowed to leverage ExUnit mechanics to do this. I have to dig into it.

Also I don't know about that, but merry XMas! :D

@dottorblaster
Copy link
Member Author

Well, this effort didn't lead to anything so I'm honestly closing this 😅

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.

Tag elixir tests into meaningful groups

3 participants