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

Consolidate various elixir makefile targets #3824

Closed
kocolosk opened this issue Nov 10, 2021 · 5 comments · Fixed by #3831
Closed

Consolidate various elixir makefile targets #3824

kocolosk opened this issue Nov 10, 2021 · 5 comments · Fixed by #3831

Comments

@kocolosk
Copy link
Member

Summary

If anyone is interested in consolidating the elixir, elixir-only, and elixir-suite into a single target I'm all ears. The first two are identical save for the MIX_ENV and COUCHDB_TEST_ADMIN_PARTY_OVERRIDE environment settings, so I'm guessing we only need one target. The last one uses a skip file instead of the @pending tag; I'm not sure we need two distinct approaches there.

Desired Behaviour

A simpler, more obvious Makefile

Additional context

Originally reported in #3818

@kocolosk
Copy link
Member Author

Some things I learned:

  • The skip.elixir file has a high degree of overlap with :pending tag, at least on main. Removing the contents of the file causes 13 additional tests to run successfully without introducing any failures.
  • mix will automatically skip any tests with a :skip tag and mark them as skipped; we are only using this in one location today.
  • We additionally configure our test runner to exclude tests with the :pending tag; we have 135 of those on main today. They are marked as excluded instead of skipped in the test output.
  • The suite.elixir file was slightly out-of-date and missing a couple of tests; it's important to regenerate this file periodically via
MIX_ENV=integration mix suite > test/elixir/test/config/suite.elixir

@iilyak
Copy link
Contributor

iilyak commented Nov 15, 2021

IRC skip.elixir was invented for a case when integrator want to disable tests without touching the upstream CouchDB source code.

@kocolosk
Copy link
Member Author

That makes sense @iilyak , thanks. I filed a PR in #3831 a few days ago to drive some improvements but didn't touch the basic skip.elixir / suite.elixir setup. I've been mulling over some ways that we might improve that and I had a few ideas. Would be curious to get your thoughts:

Option 1: Ignore suite.elixir if skip.elixir is empty

If we did this then anyone not relying on a skip.elixir file would automatically pick up new / changed tests. Folks who are relying on the skip.elixir file would still be required to maintain suite.elixir.

Option 2: Dynamically generate suite.elixir content if skip.elixir is used

With this approach no one needs to maintain suite.elixir. Calling Couch.Test.Suite.list() takes about 15 seconds on my machine; most of it is spent require-ing the .exs files, so one might imagine that the subsequent ExUnit run would be faster as a result. Anecdotally it seems 7-10 seconds faster (i.e. 5-8 seconds slower overall) but I haven't tried too hard to measure it.

Option 3: Exclude tests by name

I discovered that the module and test name are automatically injected as tags on each test. That means we could exclude tests by name using the existing skip.elixir format, but we'd have to take care to ensure globally-unique test names (today we have some identically-named tests across modules). No one need maintain suite.elixir, and there's no measurable performance penalty.

@kocolosk
Copy link
Member Author

Reopen to continue discussion of additional improvements.

@kocolosk kocolosk reopened this Nov 15, 2021
@nickva
Copy link
Contributor

nickva commented Nov 13, 2023

We settled on make elixir by renaming elixir-suite -> elixir. ExUnit never took off and the target doesn't actually run any tests we should clean that up. But otherwise, we're down to a single working target in makefile so closing as completed.

@nickva nickva closed this as completed Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants