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

Take integrations tests out of unit tests folder #2100

Merged
merged 23 commits into from
Dec 6, 2023

Conversation

safoinme
Copy link
Contributor

Describe changes

It's a bit confusing where to draw the line between integration and unit, if consider any interaction with the file system as integration that needs to be taken out but that seems a bit too much since most of these tests have very limited scope and one isolated so for that only one actual test that wasn't is the zen_server. I also would like to challenge the idea of keeping the zenml integrations tests folder under the unit instead of having it part of the integration, would it be possible to have a unit test workflow just with zenml not with all integrations installed?

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to docs, I have updated the documentation accordingly.
  • If I have added an integration, I have updated the integrations table and the corresponding website section.
  • I have added tests to cover my changes.
  • I have based my new branch on develop and the open PR is targeting develop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (add details above)

@strickvl
Copy link
Contributor

@safoinme 100% we can split up the tests that way. IMO we should have:

  1. unit tests that can be run without extra integrations installed (i.e. for core zenml functionality)
  2. integration tests that mainly deal with functionality that requires things to be installed (some will run in isolated environments with just 1 integration, and others will run in an environment that has a few integrations installed)
  3. end-to-end tests (which run in isolated environments that run full pipelines for specific integrations + also that touch deployed cloud infra)

We'll make those changes in coming weeks, I think, but this PR is a good start towards that.

@strickvl strickvl added internal To filter out internal PRs and issues tests Test suite additions or updates labels Nov 30, 2023
@strickvl
Copy link
Contributor

Previously the proportions (according to the above split) were:

End-to-End tests: 20
Integration (Functional) tests: 461
Unit tests: 573

And now with this PR the proportions have become:

End-to-End tests: 20
Integration (Functional) tests: 536
Unit tests: 498

In the end, I think you usually want to have quite a bit more unit tests than these integration tests, but in our case maybe it's a bit special given how much ZenML caters to these kinds of extra things being installed.

@safoinme
Copy link
Contributor Author

If i may add, i think there is some integrations that can be questioned or in another word can be considered unit, should we create a different PR for that an put it to review? @strickvl

@strickvl
Copy link
Contributor

@safoinme can you give an example? Also are you making sure that anything you're removing from unit tests might no longer need to be installed for unit test runners etc (so making any updates to the CI installation)? I'm not sure if that applies here.

@github-actions github-actions bot added the enhancement New feature or request label Nov 30, 2023
Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

LGTM, pending tests passing ofc. Remaining pieces to this:

  • handling caching (v important)
  • monitoring the speed / load on our runners and how we can best allocate those resources
  • Windows runners on the Azure cluster?

Copy link
Contributor

@avishniakov avishniakov left a comment

Choose a reason for hiding this comment

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

Agree with @strickvl comments

.github/workflows/ci.yml Outdated Show resolved Hide resolved
os: [ubuntu-dind-runners]
# Disabled temporarily while CI is overhauled
python-version: ["3.8", "3.9", "3.10"]
# python-version: ["3.8", "3.9", "3.10", "3.11"]
Copy link
Contributor

Choose a reason for hiding this comment

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

pls remove commented out code

@safoinme safoinme merged commit f813d9f into develop Dec 6, 2023
4 of 17 checks passed
@safoinme safoinme deleted the feature/OSS-2625-split-unit-from-integration-tests branch December 6, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internal To filter out internal PRs and issues tests Test suite additions or updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants