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

Run blackbox tests in CI #260

Closed

Conversation

michaeldiamant
Copy link
Contributor

Defines Makefile facilities to run blackbox tests from #249 as part of the build.

@michaeldiamant michaeldiamant mentioned this pull request Mar 23, 2022
11 tasks
@@ -28,6 +28,47 @@ jobs:
run: make pip
- name: Build and Test
run: make build-and-test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm mostly out of ideas here.

Locally via https://github.com/nektos/act, it works at these commits:

It's unclear to me how to otherwise debug a failed invocation (e.g. https://github.com/algorand/pyteal/runs/5664812490?check_suite_focus=true). It's quite possible I'm taking an approach that's a poor fit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for suggesting act. Really useful!!!

- name: Install required os level applications
run: |
sudo apt update -y
sudo apt install -y curl git nodejs python-is-python3 python3-pip
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of bringing these changes to a mergeable state, I think we ought to disambiguate steps needed for act.

  • In order to get act to pass, I sourced various dependencies that I believe are not needed for github.
  • Disambiguation here means creating 1 or more steps dedicated to act dependencies. Goal is to make it easier to maintain the build.
  • I think the following dependencies are act-specific though it requires a round of testing:
    • curl, nodejs, python3-pip
    • Docker install (e.g. docker-ce)
    • Setup docker compose

@tzaffi
Copy link
Contributor

tzaffi commented Mar 25, 2022

Bringing a comment over from #249 that's better placed here:

Sync'ed up offline but let's summarize my wish list regarding testing (with the caveat that clearly "testing" is better than "no testing").

From most desired to least desired (but again, even least desired is really great):

  1. have a dry run test that doesn't require running a node (a la goal clerk dryrun without the remote option)
  2. have the dry run test that uses a quickly downloadable pre-built docker image (a la docker pull algorand/stable)
  3. have a dry run that stands up the minimum necessary sandbox-like environment
  4. have a dry run that stands up a "heavier" sandbox environment similar to the one that the SDK's do

@tzaffi
Copy link
Contributor

tzaffi commented Mar 25, 2022

Bringing a comment over from #249 that's better placed here:

Sync'ed up offline but let's summarize my wish list regarding testing (with the caveat that clearly "testing" is better than "no testing").

From most desired to least desired (but again, even least desired is really great):

  1. have a dry run test that doesn't require running a node (a la goal clerk dryrun without the remote option)
  2. have the dry run test that uses a quickly downloadable pre-built docker image (a la docker pull algorand/stable)
  3. have a dry run that stands up the minimum necessary sandbox-like environment
  4. have a dry run that stands up a "heavier" sandbox environment similar to the one that the SDK's do

From my understanding the present solution is an optimized version of 3.: A simple sandbox is downloaded but it's cached so -in practice- it's almost as good as 2.

Also, we see from experience that it's taking around 5 minutes to run the tests. It's slower than the 1 minute times we had before the test, but not nearly as bad as the wait times on the SDK's and on go-algorand.

@michaeldiamant
Copy link
Contributor Author

Closing - We've evolved these techniques via algorand/graviton#1. #249 will directly bring in the CI test running.

@michaeldiamant michaeldiamant deleted the subr-blackbox_ci branch March 31, 2022 14:59
@tzaffi tzaffi mentioned this pull request Apr 14, 2022
tzaffi added a commit that referenced this pull request Apr 19, 2022
* Linting related changes:
  *`.flake8`: 
    * Based on my experience in `graviton` and a couple of extra ignores for formatting that was conflicting with `black`
    * replace `from pyteal import *` with `import pyteal as pt` in our tests, and for other files ignore the * import error on a per file basis (_this is the reason line changes number in the thousands which amounts to almost 25% of the entire codebase_)
    * Incorporate #277 
* Additional changes:
  * Removing `requirements.txt` (but keeping `docs/requirements.txt`)
  * `setup.py`: `extras_require={"development" ... ` replaces the former `requirements.txt`
  * `.github/workflows/build.yml` + `Makefile`: unify as much of the logic as possible. Needed regular `python` instead of `python-slim` in order to have `make`. `black` is applied to all python files, as before, and `flake8` is as well. `mypy` is applied to `scripts` in addition to `pyteal` (in upcoming #260 this will be expanded to `tests` as well).
  * `README.md` - applying [mardkownlint](https://github.com/DavidAnson/markdownlint) (without automation)
algoidurovic pushed a commit to algoidurovic/pyteal that referenced this pull request Apr 20, 2022
* Linting related changes:
  *`.flake8`: 
    * Based on my experience in `graviton` and a couple of extra ignores for formatting that was conflicting with `black`
    * replace `from pyteal import *` with `import pyteal as pt` in our tests, and for other files ignore the * import error on a per file basis (_this is the reason line changes number in the thousands which amounts to almost 25% of the entire codebase_)
    * Incorporate algorand#277 
* Additional changes:
  * Removing `requirements.txt` (but keeping `docs/requirements.txt`)
  * `setup.py`: `extras_require={"development" ... ` replaces the former `requirements.txt`
  * `.github/workflows/build.yml` + `Makefile`: unify as much of the logic as possible. Needed regular `python` instead of `python-slim` in order to have `make`. `black` is applied to all python files, as before, and `flake8` is as well. `mypy` is applied to `scripts` in addition to `pyteal` (in upcoming algorand#260 this will be expanded to `tests` as well).
  * `README.md` - applying [mardkownlint](https://github.com/DavidAnson/markdownlint) (without automation)
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

2 participants