repo2docker icon indicating copy to clipboard operation
repo2docker copied to clipboard

ci: refactor julia/r/conda tests - now ~25 min instead of ~50 min

Open consideRatio opened this issue 3 years ago • 3 comments

Previously the CI system took ~50 minutes. The tests in the folders tests/conda, tests/julia, and tests/r all took longer than 25 minutes but are now reduced to about ~25 minutes or less.

Change description

  • Deleted and merged tests Test coverage is retained well I think, even expanded towards modern versions. Overall the following principles guided what I retained.
    • oldest - py2, py3.5, r3.6?, julia 1.0.2? (Project.toml buildpack), julia 0.6.4? (REQUIRE buildpack) See #1192
    • default - py3.7, r4.2, julia latest (Project.toml buildpack), julia 0.6.4 (REQUIRE buildpack) See #1190
    • latest - py3.10, r4.2, julia latest (Project.toml buildpack)
    • before and after - R with mran and R with rspm, julia environment variables after 1.0+
    • pinnability - ensure language versions can be pinned
    • functionality - binder folder, description file, requirements file, etc.
  • Renamed test folders, added/updated READMEs

Test changes

Changes to tests/conda

After Before Note
py35-binder-dir binder-dir py37 Makes an old version the oldest version, and merges in the binder directory test
py310-requirements-file py38 requirements Makes a less old version the latest version, and merges in the requirements file test
py2 simple-py2 Renamed
r-postbuild r-unspecified default-env simple r4.0 r4.1 Merges in the postBuild file test (default-env), merges tests of modern R versions into the latest version (r4.0, r4.1), merges tests of the default Python version (simple)
r3.6-extra-args repo-path r3.6 Merges extra-args.yml file test with r3.6 pinning test
- channel-dep A deleted test, testing functionality part of mamba since 0.6 introduced more than two years ago

Changes to tests/r

After Before Note
r3.6-mran r3.6 Renamed
r4.0-mran mran, r4.0 Merges similar tests
r4.0-rspm r4.0-rspm, r4.1 Merges r4.0 and r4.1,
r-rspm-apt apt, simple Merges in the apt.txt file test
r-rspm-description description-file Renamed

Changes to tests/julia

After Before Note
project julia_version-default Merges in the requirements file test
project-1.0.2 julia_version-1.0.2 Renamed
require julialegacy_version-0.6.3 The default version for require is 0.6.4, and can be used to test the oldest version.
require-1-requirements pyplot-requirements julialegacy_version-1 julialegacy_version-1.0 julialegacy_version-1.1 Merged similar tests

Before

image

After

image

consideRatio avatar Oct 09 '22 11:10 consideRatio

To help with review would you mind explaining the overall thoughts behind your changes? For example, what's your thinking behind bumping versions instead of leaving them as they are?

I think we should keep intermediate versions, for example an old bug was included installing the wrong version of R when 4.0 was requested https://github.com/jupyterhub/repo2docker/issues/1077 so this should be kept as a regression test.

manics avatar Oct 09 '22 14:10 manics

@manics I've updated the PR description with a declaration of the principles that has guided me when refactoring this! I think we still cover #1077 in the R buildback by testing both before (mran) and after (rspm) situations.

consideRatio avatar Oct 09 '22 15:10 consideRatio

Sorry for the noise in this PR @manics @yuvipanda, with a lot of force pushes and too early transition from a draft PR to a ready for review PR.

At this point, it is ready for review though!

consideRatio avatar Oct 10 '22 09:10 consideRatio

Thanks for the additional description- it makes sense, though I'm still struggling to review the actual changes. I'm also concerned we're going to loose some of the information on the purposes of individual tests, which is a problem for any future refactoring.

I noticed you added a few READMEs in some tests, what do you think about adding READMEs for each group of tests instead/as well, e.g.

tests/conda/README.md
tests/julia/README.md
tests/r/README.md

where each file describes how the subtests in that directory combines multiple factors, such that they cover all combinations?

manics avatar Oct 14 '22 18:10 manics

I'm still struggling to review the actual changes.

I'm sorry about this, if you have a suggestion on what I can do to help let me know. I have considered breaking apart this refactoring PR into pieces such as one for the conda buildpacks tests, and one for julia, and one for R. Doing that though would make common aspects need to be discussed in separate PRs, such as the README suggestion etc and I figure it wouldn't be very helpful.

I'm also concerned we're going to loose some of the information on the purposes of individual tests, which is a problem for any future refactoring.

Having worked with these files many hours now, I'm quite confident this is an improvement with regards to future refactoring work. The git history will be obfuscated by this refactor, but the documentation surrounding the tests will be accurate and updated. I've been surprised several times by un

I noticed you added a few READMEs in some tests, what do you think about adding READMEs for each group of tests instead/as well, e.g.

tests/conda/README.md
tests/julia/README.md
tests/r/README.md

where each file describes how the subtests in that directory combines multiple factors, such that they cover all combinations?

:100: implemented now!

consideRatio avatar Oct 16 '22 09:10 consideRatio

LET'S GOOOOO! THANK YOU SO MUCH @MANICS AND @CONSIDERATIO!

yuvipanda avatar Oct 18 '22 05:10 yuvipanda

let it be known that I tried to upper case your usernames too ^ but github autocorrected them

yuvipanda avatar Oct 18 '22 05:10 yuvipanda