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

fix: #1387 persist include dir after running pytest #1459

Merged

Conversation

zach-nicoll-wcq
Copy link
Contributor

@zach-nicoll-wcq zach-nicoll-wcq commented Nov 27, 2023

Description

After running astro dev pytest, the coverage reports created with the include dir are not being persisted outside of the test container.

This is a problem, because the coverage reports cannot then be used to update PR comments etc. with the report results. Essentially, coverage reports are useless.

In the current state, this statement is false:

If your test generates artifacts, such as code coverage reports, you can output the artifacts to the include folder of your Astro project so they can be accessed after the test has finished.

This PR makes the above statement true, copying the include folder on the host machine after the Pytest run completes.

🎟 Issue(s)

#1387

🧪 Functional Testing

  1. astro dev init
  2. Add pytest-cov==4.1.0 to requirements.txt (or any other pytest-cov version)
  3. astro dev pytest --args "--cov --cov-report=xml:include/coverage.xml --cov-report=html:include/html"
  4. Observe that the include dir is populated once the tests succeed

📸 Screenshots

1387

📋 Checklist

  • Rebased from the main (or release if patching) branch (before testing)
  • Ran make test before taking out of draft
  • Ran make lint before taking out of draft
  • Added/updated applicable tests
  • Tested against Astro-API (if necessary).
  • Tested against Houston-API and Astronomer (if necessary).
  • Communicated to/tagged owners of respective clients potentially impacted by these changes.
  • Updated any related documentation

@sunkickr
Copy link
Contributor

did you try astro dev pytest --args "--cov --cov-report xml:include/coverage.xml"?

@sunkickr
Copy link
Contributor

The way volumes is handled by the pytest command changed which must have broke the functionality described in docs. we can add the cp command if needed. The code within if htmlReport is used by the astro dev upgrade-test command to create an html report using pytest-html. That code would need to stay

@zach-nicoll-wcq
Copy link
Contributor Author

did you try astro dev pytest --args "--cov --cov-report xml:include/coverage.xml"?

Yes, if you see my functional testing steps, this is one of them to verify the functionality. Alas, it does not work in the current state.

The way volumes is handled by the pytest command changed which must have broke the functionality described in docs. we can add the cp command if needed. The code within if htmlReport is used by the astro dev upgrade-test command to create an html report using pytest-html. That code would need to stay

I see - to simplify the process, would it be easier to just put the dag-test-report.html in the include dir, and then perform a copy? That way everything can be done at once.

@sunkickr
Copy link
Contributor

@zach-nicoll-wcq we copy the test report into a specific testHomeDirectory which is separate from include. it makes sense to me to continue to only copy the html-report into testHomeDirectory for the upgrade-test command and add this more general copy of the include folder every time the pytest function is called

@zach-nicoll-wcq zach-nicoll-wcq force-pushed the issue-1387-persistent-include-dir branch 2 times, most recently from 7273e93 to 4e67ac0 Compare December 1, 2023 01:35
@zach-nicoll-wcq
Copy link
Contributor Author

@zach-nicoll-wcq we copy the test report into a specific testHomeDirectory which is separate from include. it makes sense to me to continue to only copy the html-report into testHomeDirectory for the upgrade-test command and add this more general copy of the include folder every time the pytest function is called

@sunkickr roger that - I have amended PR to perform the copy after the html report logic is run.

I also noticed that the container shutdown logic was repeated, so I refactored the error handling at the end of the Pytest function to log the docker errors, and only remove the docker container in a single place.

@zach-nicoll-wcq zach-nicoll-wcq force-pushed the issue-1387-persistent-include-dir branch from 4e67ac0 to 1b4aa27 Compare December 5, 2023 03:47
@zach-nicoll-wcq
Copy link
Contributor Author

@sunkickr @neel-astro @kushalmalani any movement on this? Would love to start using this functionality in my own projects.

// delete container
err = cmdExec(dockerCommand, nil, stderr, "rm", "astro-pytest")
docErr = cmdExec(dockerCommand, nil, stderr, "rm", "astro-pytest")
if err != nil {
Copy link

Choose a reason for hiding this comment

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

if docErr != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SebOpo have amended :)

@zach-nicoll-wcq zach-nicoll-wcq force-pushed the issue-1387-persistent-include-dir branch from 790bb3f to 94940f2 Compare December 7, 2023 02:24
@zach-nicoll-wcq
Copy link
Contributor Author

@sunkickr @neel-astro @kushalmalani @SebOpo

Any movement? This feels like a very easy win for the project, and will align the functionality to what people expect from the docs...

@sunkickr
Copy link
Contributor

Worried about is verifying that other command that use the function still work:

  • astro dev pytest without using the --args flag
  • astro dev upgrade-test command

I'll just verify before release

@sunkickr sunkickr merged commit 6810929 into astronomer:main Dec 14, 2023
1 check passed
@zach-nicoll-wcq zach-nicoll-wcq deleted the issue-1387-persistent-include-dir branch January 4, 2024 00:05
sunkickr added a commit that referenced this pull request Jan 29, 2024
* fix astro deploy when dag deploy is off

* fix deploy flow

* add description

* fix dags

* fix dags

* fix dags

* fix dags

* fix lint

* fix lint

* fix dags

* fix lint

* fix lint

* list and selection process to core

* list/selection to core

* deployment list & selection to CORE

* broken but working on it

* update mocks

* fix log

* fix lint

* fixing worker queues

* worker-queue

* worker-queue work

* able to compile

* get create command working

* manually tested commands

* fix deploy

* fix lint

* remove commented out code

* fix some tests

* fix some deployment tests

* fix unit tests

* fix unit tests

* fix unit tests

* fix tests

* fix tests

* fix tests

* fix update tests

* fix test

* fix test

* fix test

* fix inspect and worker queue tests

* fix inspect

* fix inspect tests

* fix update tests

* fix variable modify

* fix setup tests

* fix root

* remove some comments

* remove comments

* fix some lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix broken tests

* fix broken tests

* fix org switch

* fix org switch

* fix deploy tests

* fix setup tests

* Fixing tests

* Fixing tests

* test cloud to local

* add tests

* add tests

* add tests

* add tests

* fix test

* add test

* add test

* update deployment update tests

* add test

* fix lint

* fix test

* fix test

* add inspect test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix lint

* fix dag deploy enabled

* add tests

* fix test

* fix test

* add test

* add test

* fix lint

* clean code

* fix mock

* fix test

* fix test

* update audit logs command

* migrate audit logs

* remove astrohub api

* list and selection process to core

* list/selection to core

* deployment list & selection to CORE

* broken but working on it

* fixing worker queues

* worker-queue

* worker-queue work

* able to compile

* get create command working

* manually tested commands

* fix deploy

* remove commented out code

* fix some tests

* fix some deployment tests

* fix unit tests

* fix unit tests

* fix unit tests

* fix tests

* fix tests

* fix tests

* fix update tests

* fix test

* fix test

* fix test

* fix inspect and worker queue tests

* fix inspect

* fix inspect tests

* fix update tests

* fix variable modify

* fix setup tests

* fix root

* remove some comments

* remove comments

* fix some lint

* Fixing tests

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* fix lint

* migrate deploy to core (#1411)

* migrate deploy to core

* fix lint

* fix lint

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix astro deploy when dag deploy is off

* fix deploy flow

* add description

* fix dags

* fix dags

* fix dags

* fix dags

* fix lint

* fix lint

* fix dags

* fix lint

* fix lint

* update mocks

* fix log

* fix lint

* remove pull (#1431)

* fix dag bundle copy (#1433)

* pinning version 1.20.0 (#1434)

* Revert "pinning version 1.20.0 (#1434)" (#1435)

This reverts commit cc04467.

* Releasing 1.20.0 (#1436)

* pinning version back to 1.19.3 (#1439)

* Set default version back to v1.20.0 (#1441)

Now that the issue has been fixed
(https://status.astronomer.io/incidents/4z4r646s6mkx) we can make this
version the default again

* catch API error (#1440)

* Fix deployment connections and airflow variable commands (#1442)

* Fix deployment connections and airflow variable commands

* fixing lint

* Fix variable name

* Fix url in test

* Fix url in test

* pin 1.20.1 (#1443)

* allow ASTRO_DOMAIN env var to override context (#1451)

* Updating error message (#1456)

* [pre-commit.ci] pre-commit autoupdate (#1447)

updates:
- [github.com/psf/black: 23.10.1 → 23.11.0](psf/black@23.10.1...23.11.0)
- [github.com/adrienverge/yamllint.git: v1.32.0 → v1.33.0](https://github.com/adrienverge/yamllint.git/compare/v1.32.0...v1.33.0)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Fix deployment file create for kube deployment (#1454)

* fix dpeloyment file create for kube deployment

* undo comment

* undelete file

* fix test

* fix test

* add --image flag for image only deploy (#1446)

* add --image flag for image only deploy

* don't send tarball on image only

* image only deploy works

* image only deploy works

* fix lint

* add errors

* Fix issues from bug bash (#1460)

* Fix issues from bug bash

* refactor code

* Update cloud/deploy/deploy.go

Co-authored-by: kushalmalani <kushal@astronomer.io>

* fix test

* fix test

---------

Co-authored-by: kushalmalani <kushal@astronomer.io>

* override the domain in more places (#1463)

* FIx astro deploy <deployment-id> --image (#1464)

* astro deploy <deployment-id> --image does not work

* update tests

* update tests

* pin 1.21.0 (#1465)

* Add v1beta1 API Clients (#1444)

* add v1beta1 api

* fix GetPublicRESTAPIURL

* fix GetPublicRESTAPIURL

* fix GetPublicRESTAPIURL

* made changes from code review

* fix error

* empty commit

* empty commit

* Update astro-client-platform-core/client.test.go

Co-authored-by: Vandy Liu <33995460+vandyliu@users.noreply.github.com>

* Update astro-client-iam-core/client.test.go

Co-authored-by: Vandy Liu <33995460+vandyliu@users.noreply.github.com>

---------

Co-authored-by: Vandy Liu <33995460+vandyliu@users.noreply.github.com>

* Set CLI environment management flag default to true (#1467)

* fix: #1387 persist include dir after running pytest (#1459)

* Migrate organization to use core (#1450)

* add v1beta1 api

* fix GetPublicRESTAPIURL

* fix GetPublicRESTAPIURL

* fix GetPublicRESTAPIURL

* migrate organization to v1beta1

* made changes from code review

* fix error

* empty commit

* Update root.go

* gofumpt

* Update setup.go

* Update setup.go

* Update setup_test.go

* fix pytest

---------

Co-authored-by: David Koenitzer <davidkoenit@gmail.com>

* [pre-commit.ci] pre-commit autoupdate (#1470)

updates:
- [github.com/psf/black: 23.11.0 → 23.12.0](psf/black@23.11.0...23.12.0)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Fixing unit tests to use LocalPlatform Config (#1471)

* Fixing unit tests to use LocalPlatform Config

* Fixing tests

* Fixing tests

* Fixing tests

* fix lint

* fix broken tests

* fix broken tests

* fix org switch

* fix org switch

* fix deploy tests

* fix setup tests

* Fixing tests

* test cloud to local

* add tests

* add tests

* add tests

* add tests

* fix test

* add test

* add test

* update deployment update tests

* add test

* fix lint

* fix test

* fix test

* add inspect test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix lint

* Bump github.com/containerd/containerd from 1.5.18 to 1.6.26 (#1472)

Bumps [github.com/containerd/containerd](https://github.com/containerd/containerd) from 1.5.18 to 1.6.26.
- [Release notes](https://github.com/containerd/containerd/releases)
- [Changelog](https://github.com/containerd/containerd/blob/main/RELEASES.md)
- [Commits](containerd/containerd@v1.5.18...v1.6.26)

---
updated-dependencies:
- dependency-name: github.com/containerd/containerd
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* [pre-commit.ci] pre-commit autoupdate (#1474)

updates:
- [github.com/psf/black: 23.12.0 → 23.12.1](psf/black@23.12.0...23.12.1)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Bump golang.org/x/crypto from 0.14.0 to 0.17.0 (#1473)

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.14.0 to 0.17.0.
- [Commits](golang/crypto@v0.14.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix dag deploy enabled

* add tests

* fix test

* fix test

* add test

* add test

* fix lint

* Updating code owners (#1480)

* Updating code owners

* adding individual members

* Updating code owners (#1482)

* Fix login and organization list/switch (#1476)

* fix not finshed

* fix login, organization list/switch

* fix version

* clean code

* Build Secrets Flag (#1478)

* add build secrets flag

* fix tests

* fix lint

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* fix test

* add test

* add test

* remove debugging messages

* Update cmd/airflow.go

Co-authored-by: Jake Witz <74574233+jwitz@users.noreply.github.com>

---------

Co-authored-by: Jake Witz <74574233+jwitz@users.noreply.github.com>

* fix mock

* fix test

* update audit logs command

* migrate audit logs

* remove astrohub api

* fix errors

* fix lint

* fix lint

* fix test

* fix lint and test

* audit logs test

* fix test

* fix lint

* fix merge issue

* fix lint

* remove debug comments

* remove debug comment

* remove audit logs files

* Update cloud/organization/organization.go

Co-authored-by: Vandy Liu <33995460+vandyliu@users.noreply.github.com>

* update tests

* fix lint

* fix tests

* fix lint

* add assertions

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Kushal Malani <kushal@astronomer.io>
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
Co-authored-by: Mehul Goyal <mehul.goyal@astronomer.io>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Vandy Liu <33995460+vandyliu@users.noreply.github.com>
Co-authored-by: Julian LaNeve <lanevejulian@gmail.com>
Co-authored-by: Zach Nicoll <151692348+zach-nicoll-wcq@users.noreply.github.com>
Co-authored-by: Shalin Patel <42554035+PatelShalin@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jake Witz <74574233+jwitz@users.noreply.github.com>
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.

None yet

3 participants