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.

Sign up for GitHub

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

Jump to bottom

Switch to an environment variable for enabling Oracle tests. #9511

Merged
merged 5 commits into from
Apr 27, 2021
Merged

Switch to an environment variable for enabling Oracle tests. #9511

merged 5 commits into from
Apr 27, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 27, 2021

This allows developers who don't have access to Oracle to build the tests.

It also stops IntelliJ from complaining that the targets don't exist.

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@ghost ghost requested review from cocreature, dasormeter and aherrmann-da April 27, 2021 09:46
moritzkiefer-da
moritzkiefer-da reviewed Apr 27, 2021
View reviewed changes
ledger-service/http-json-oracle/BUILD.bazel Outdated
@@ -60,4 +57,4 @@ da_scala_test(
"//libs-scala/oracle-testing",
"//libs-scala/ports",
],
)
) if oracle_testing else None
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead set it to manual? testing that it builds still seems useful.

ledger/participant-integration-api/BUILD.bazel
)

da_scala_test_suite(
# The "manual" test tag doesn't seem to work with `da_scala_test_suite`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m confused. You switched from da_scala_test_suite but then you’re still not using manual. Can we do one or the other?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that was my original plan and then I switched it. I'll put it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’d prefer the manual option and fixing da_scala_test_suite.

Copy link
Author

Choose a reason for hiding this comment

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

I did try to fix da_scala_test_suite but I can't find the problem. Can you see one?

I think it might be an issue in scala_test_suite, but I can't be sure.

Copy link
Author

Choose a reason for hiding this comment

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

Switched to manual execution with the tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, I thought it might not pass the tag along but doesn’t seem to be it. Fine to leave it out of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the implementation of scala_test_suite, it does forward kwargs to the individual scala_test targets, but it doesn't forward any tags to the final test_suite target. So, the final test_suite target will still be non-manual. I'd say that's a bug in rules_scala.

triggers/service/BUILD.bazel
@@ -239,19 +240,16 @@ da_scala_test_suite(
deps = test_deps,
)

da_scala_test_suite(
# The "manual" test tag doesn't seem to work with `da_scala_test_suite`.
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing

cocreature
cocreature approved these changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Thanks!

cocreature
cocreature reviewed Apr 27, 2021
View reviewed changes
triggers/service/BUILD.bazel
@@ -239,14 +240,12 @@ da_scala_test_suite(
deps = test_deps,
)

da_scala_test_suite(
# The "manual" test tag doesn't seem to work with `da_scala_test_suite`.
da_scala_test(
Copy link
Contributor

Choose a reason for hiding this comment

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

there are a bunch of unused deps here. Not caused by your PR but da_scala_test_suite (deliberately) disables unused dep checks so now it blows up. Should be able to just kill them.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, missed those somehow. I purged the ones in participant-integration-api.

Not sure how bazel build //... didn't catch that.

Copy link
Author

Choose a reason for hiding this comment

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

Seems I needed to run DAML_ORACLE_TESTING=true bazel build //..., even with the change to the manual tags. I guess they don't get built otherwise.

@ghost ghost added the automerge label Apr 27, 2021
@mergify mergify bot merged commit 8cd3658 into main Apr 27, 2021
@mergify mergify bot deleted the samir/oracle-testing branch April 27, 2021 11:50
@aherrmann aherrmann mentioned this pull request Apr 27, 2021
Merged
6 tasks
azure-pipelines bot pushed a commit that referenced this pull request Apr 28, 2021
release 1.13.0-snapshot.20210428.6782.0.e1e878a5
a9ed266 
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@nickchapman-da is in charge of this release.

Commit log:
```
e1e878a Simplify opt-in/out of Oracle when building (#9515)
d761853 KVL-874 Add unit tests for Telemetry (#9500)
c567680 Support rollback nodes in PostCommitValidation (#9501)
a335ee8 Support rollback nodes in KeyValueCommitting.submissionOutputs (#9512)
8747b3d Support rollback nodes in TransactionIndexing (#9506)
c4cf3c9 kvutils: Log a missing input state warning without the stack trace. (#9513)
4e712a0 add oracle option for json-api perf runner (#9492)
8cd3658 Switch to an environment variable for enabling Oracle tests. (#9511)
e39c20e update GPG public key (#9488)
782109d update LATEST (#9508)
3e66611 Nest stakeholders in contracts table as JSON arrays (#9484)
aecdc2a update NOTICES file (#9507)
```
Changelog:
```
- [JSON-API Perf] ``--query-store-index=postgres`` must be passed
  to select PostgreSQL query store performance testing; ``true``
  and ``yes`` are no longer supported.
  See `issue #9492 <https://github.com/digital-asset/daml/pull/9492>`__.
```

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

@dasormeter dasormeter Awaiting requested review from dasormeter

@aherrmann-da aherrmann-da Awaiting requested review from aherrmann-da

@garyverhaegen-da garyverhaegen-da Awaiting requested review from garyverhaegen-da

@gerolf-da gerolf-da Awaiting requested review from gerolf-da

@mziolekda mziolekda Awaiting requested review from mziolekda

@nmarton-da nmarton-da Awaiting requested review from nmarton-da

@rautenrieth-da rautenrieth-da Awaiting requested review from rautenrieth-da

@S11001001 S11001001 Awaiting requested review from S11001001

@stefanobaghino-da stefanobaghino-da Awaiting requested review from stefanobaghino-da

@tudor-da tudor-da Awaiting requested review from tudor-da

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cocreature @aherrmann-da @moritzkiefer-da @SamirTalwar