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

Conditionally skip Oracle tests #609

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

willmostly
Copy link
Contributor

@willmostly willmostly commented Feb 3, 2025

The Oracle test container runs very slowly on non x86 architectures, which causes tests to fail. These tests will be skipped unless they are running in Github Actions, on an x86_64, or explicitly enabled by setting the env var TG_RUN_ORACLE_TESTS=true.

Description

Additional context and related issues

#605

Release notes

(x ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Feb 3, 2025
@willmostly
Copy link
Contributor Author

Manually verified that the Oracle tests run in CI:

[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 40.74 s -- in io.trino.gateway.ha.router.TestQueryHistoryManagerOracle
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 25.67 s -- in io.trino.gateway.ha.persistence.TestDatabaseMigrationsOracle

but are skipped on my M1 Macbook.

@willmostly willmostly force-pushed the will/conditionally-skip-oracle branch from 90e1cb1 to 711e223 Compare February 3, 2025 23:09
private static OracleContainer getOracleContainer()
{
// reference: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables
assumeTrue("true".equals(System.getenv("GITHUB_ACTIONS"))
Copy link
Member

Choose a reason for hiding this comment

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

I think we should extract this into a shared method .. truly ugly otherwise

Copy link
Member

Choose a reason for hiding this comment

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

Also .. we should add this info to the development docs

Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please find a different docker image instead.

@mosabua
Copy link
Member

mosabua commented Feb 4, 2025

Please find a different docker image instead.

Oracle or anyone else at this stage does NOT publish an image that is suitable for ARM. The same problem with the container image exists in Trino itself and we think this is a good approach to allow local testing and development on ARM-based machines .. currently without this approach you have to skip the tests

@ebyhr
Copy link
Member

ebyhr commented Feb 4, 2025

That's not true https://blogs.oracle.com/database/post/announcing-oracle-database-23ai-free-container-images-for-armbased-apple-macbook-computers. I would recommend looking into this image's license.

@willmostly
Copy link
Contributor Author

Thanks @ebyhr, I was not aware of that image. The license looks harmless to me, but this isn't my area of expertise. @mosabua thoughts?

@willmostly
Copy link
Contributor Author

@willmostly willmostly closed this Feb 4, 2025
@mosabua mosabua reopened this Feb 4, 2025
@mosabua
Copy link
Member

mosabua commented Feb 4, 2025

That image is only the latest version of Oracle and from the tutorial it requires an account with the Oracle container registry with a whole lot of restrictions.

I dont know if we can use it with GitHub actions either... we can look into it but for now I would think we should merge this PR after cleaning it up

@mosabua
Copy link
Member

mosabua commented Feb 4, 2025

There is lots more info at https://container-registry.oracle.com/ .. which we have to look at for more info. At first glance I saw no docs for accessing older versions

@mosabua
Copy link
Member

mosabua commented Feb 4, 2025

Also .. maybe @wendigo or someone else knows why we use the container we use here .. I will ask

@willmostly willmostly force-pushed the will/conditionally-skip-oracle branch from 711e223 to 3e6edf4 Compare February 4, 2025 21:51
The Oracle test container runs very slowly on non x86 architectures, which
causes tests to fail. These tests will be skipped unless they are running
in Github Actions, on an x86_64, or explicitly enabled by setting the
env var TG_RUN_ORACLE_TESTS=true
@willmostly willmostly force-pushed the will/conditionally-skip-oracle branch from 3e6edf4 to ad771bf Compare February 4, 2025 22:00
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks @willmostly

@mosabua mosabua merged commit e308c93 into trinodb:main Feb 5, 2025
2 checks passed
@github-actions github-actions bot added this to the 14 milestone Feb 5, 2025
@ebyhr
Copy link
Member

ebyhr commented Feb 5, 2025

@mosabua I was not convinced with this change. I'm not sure why you merged this PR even I did requested changes.

@mosabua
Copy link
Member

mosabua commented Feb 5, 2025

Apologies @ebyhr. We discussed this in the dev sync this morning and agreed to merge it for now. It is currently a pain to build locally and it will also potentially cause an issue for the release build that @martint will want to run as soon as we get things ready. As such we decided to merge this and then potentially follow up with adjustments. I still have to get around to write the meeting notes .. hopefully after I get the 470 release notes completed.

@ebyhr
Copy link
Member

ebyhr commented Feb 5, 2025

@mosabua I don't think this change is required for "build". Does Martin run all tests during the release process?

@mosabua
Copy link
Member

mosabua commented Feb 5, 2025

They run by default in the normal build so my local build without this currently fails. Not sure about release build .. but typically they do run unless we have that deactivate in airbase / the release profile.

See #605

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants