Skip to content

Use matrix for db recorder tests#87640

Merged
epenet merged 13 commits into
home-assistant:devfrom
epenet:mariadb
Feb 8, 2023
Merged

Use matrix for db recorder tests#87640
epenet merged 13 commits into
home-assistant:devfrom
epenet:mariadb

Conversation

@epenet
Copy link
Copy Markdown
Contributor

@epenet epenet commented Feb 7, 2023

Proposed change

Should resolve
image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant home-assistant Bot added cla-signed code-quality small-pr PRs with less than 30 lines. labels Feb 7, 2023
Comment thread .github/workflows/ci.yaml Outdated
@frenck
Copy link
Copy Markdown
Member

frenck commented Feb 7, 2023

Mark mariadb as success not skip on partial builds

I do not agree with the stance in this PR. It is skipped, did not actually run. Making it look like it ran gives the wrong expectation / sends the wrong message.

I don't like skipped jobs either (I wish we could just not show them), but just marking them as passed is not right either.

@epenet epenet marked this pull request as draft February 7, 2023 15:16
@epenet epenet changed the title Mark mariadb as success not skip on partial builds Ignore db tests on partial builds Feb 7, 2023
@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Feb 7, 2023

I do not agree with the stance in this PR. It is skipped, did not actually run. Making it look like it ran gives the wrong expectation / sends the wrong message.

I don't like skipped jobs either (I wish we could just not show them), but just marking them as passed is not right either.

I think I have found a solution, using a matrix.

@epenet epenet marked this pull request as ready for review February 7, 2023 16:35
Comment thread .github/workflows/ci.yaml Outdated
@epenet epenet mentioned this pull request Feb 7, 2023
19 tasks
@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Feb 7, 2023

As s bonus, this new flavour would enable the ability to test different versions of postgre and/or mariadb.

@epenet epenet changed the title Ignore db tests on partial builds Use matrix for db recorder tests Feb 7, 2023
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Feb 8, 2023

It would be nice to get visibility on which lines are covered with MariaDB/Postgresql since we have PRs like this that look like they have no coverage for those paths but the test will cover some of those lines
#87583
#87321

@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Feb 8, 2023

I'll take a look tomorrow, but the coverage should get uploaded

[2023-02-08T17:06:36.816Z] ['info'] => Found 12 possible coverage files:
  coverage-3.10-1/coverage.xml
  coverage-3.10-10/coverage.xml
  coverage-3.10-2/coverage.xml
  coverage-3.10-3/coverage.xml
  coverage-3.10-4/coverage.xml
  coverage-3.10-5/coverage.xml
  coverage-3.10-6/coverage.xml
  coverage-3.10-7/coverage.xml
  coverage-3.10-8/coverage.xml
  coverage-3.10-9/coverage.xml
  coverage-3.10-mariadb/coverage.xml
  coverage-3.10-postgresql/coverage.xml

@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Feb 8, 2023

It would be nice to get visibility on which lines are covered with MariaDB/Postgresql since we have PRs like this that look like they have no coverage for those paths but the test will cover some of those lines #87583 #87321

The reason I removed from needs is so that it doesn't block coverage upload on partial runs.
For full runs the recorder test will normally finish before the full suite so they'll be included anyway.

Comment thread .github/workflows/ci.yaml
HA_SHORT_VERSION: 2023.3
DEFAULT_PYTHON: "3.10"
ALL_PYTHON_VERSIONS: "['3.10']"
MARIADB_VERSIONS: "['mariadb:10.9.3']"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unrelated to this PR but we should probably be testing with 10.6.10 since that is what ships with the addon

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍
We can do that in a follow-up PR

Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Updated solution LGTM 👍

@epenet
Copy link
Copy Markdown
Contributor Author

epenet commented Feb 8, 2023

@frenck any last comments before I merge?

@epenet epenet merged commit a31bd49 into home-assistant:dev Feb 8, 2023
@epenet epenet deleted the mariadb branch February 8, 2023 21:21
@frenck
Copy link
Copy Markdown
Member

frenck commented Feb 8, 2023

@epenet Sorry, no, I don't think so :) My main issue with the proposition seems to be addressed, so happy 👍

@github-actions github-actions Bot locked and limited conversation to collaborators Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed code-quality small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants