Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Traffic Ops integration tests#4959

Merged
jhg03a merged 59 commits intoapache:masterfrom
ocket8888:ciab-less-tests
Aug 18, 2020
Merged

Traffic Ops integration tests#4959
jhg03a merged 59 commits intoapache:masterfrom
ocket8888:ciab-less-tests

Conversation

@ocket8888
Copy link
Copy Markdown
Contributor

What does this PR (Pull Request) do?

  • This PR is not related to any Issue

This PR adds running the API tests to Pull Requests, any time a source file for Traffic Ops, one of its Go clients, or the Go client/API integration tests changes.

Which Traffic Control components are affected by this PR?

None.

What is the best way to verify this PR?

Observe the test output.

The following criteria are ALL met by this PR

  • This PR includes tests
  • Documentation is unnecessary
  • An update to CHANGELOG.md is not necessary
  • This PR includes any and all required license headers
  • This PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@zrhoffman
Copy link
Copy Markdown
Member

How hard would it be to get junit reports for v1, v2, and v3 as artifacts?

@ocket8888
Copy link
Copy Markdown
Contributor Author

Probably not too hard. But I don't know how yet.

@zrhoffman
Copy link
Copy Markdown
Member

Running v1, v2, and v3 in separate steps would be nice so we can tell at a glance which API version had failing tests.

@ocket8888
Copy link
Copy Markdown
Contributor Author

If they're separate steps, v2 and v3 tests won't run if the preceding step - v1 tests - fails. If they're separate jobs then we duplicate a lot of work, which can cause it to slow down because it's waiting for executors. I think a better solution would be to use the reporting API to provide detailed error descriptions.

@zrhoffman
Copy link
Copy Markdown
Member

If they're separate steps, v2 and v3 tests won't run if the preceding step - v1 tests - fails.

By default, sure, but if: ${{ always() }} covers that case, right?

@ocket8888
Copy link
Copy Markdown
Contributor Author

We're about to find out

@ocket8888
Copy link
Copy Markdown
Contributor Author

I wound up separating out the database setup to alleviate what became a much longer runtime, so instead of if: ${{ always() }} it checks that that step, specifically, succeeded. But it's working as separate steps now.

Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

From my testing, checking for always() was still necessary if the tests failed for a previous API version.

Comment thread .github/actions/to-integration-tests/Dockerfile Outdated
Comment thread .github/workflows/traffic ops.yml Outdated
Comment thread .github/actions/todb-init/Dockerfile Outdated
envsubst </cdn.json >cdn.conf
mv /database.json ./database.conf

./traffic_ops_golang --cfg ./cdn.conf --dbcfg ./database.conf >out.log 2>err.log &
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.

Should stdout and stderr go to the same file so we have context for errors and warnings? Errors will still be pretty easy to find by searching for ERROR:.

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.

stdout won't contain anything too helpful, I pretty much just do that to avoid writing to closed file descriptors, just in case. info, event, and debug logging is piped to /dev/null by the logging configuration. I tried including those in stderr as well, but the resulting logs after the tests finished were so large my browser refused to load the checks page that printed them. One day, we can put the entire log in a build artifact, so at that point we don't need to worry about what the GH UI can handle and can include everything in one stream.

Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

  • API tests are in separate job steps and the results are easy to find without digging into the logs
  • v2 and v3 will still run if a previous API test version failed
  • JSON and YAML are now read from external files

We could always improve how we show the logs of TO itself, but none of that is directly related to the API tests and can be added in the future IMO.

Looks good to me, this will help stability and PR review considerably.

@jhg03a jhg03a merged commit 6a20d6f into apache:master Aug 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automation related to automated testing/deployment/packaging etc. new feature A new feature, capability or behavior tests related to tests and/or testing infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants