-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 TS SDK tests against local testnets built from prod branches, split API and Rust node API client checks into separate job #8769
Conversation
7b1e077
to
55224ee
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6d08de5
to
7450405
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a34efaf
to
aad288d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BEAUTIFUL!
@@ -15,13 +15,22 @@ const publisher = new AptosAccount( | |||
const alice = new AptosAccount(); | |||
const bob = new AptosAccount(); | |||
let fungibleAssetMetadataAddress = ""; | |||
|
|||
// Do not run these tests if the network is testnet / mainnet right now. | |||
let maybe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
// CI, it is essential that TMPDIR is set to a directory that can actually be mounted. | ||
// Learn more here: https://stackoverflow.com/a/76523941/3846032. | ||
console.log("---creating temporary directory for ANS code---"); | ||
let tempDir = execSync("mktemp -d").toString("utf8").trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mktemp
create a temp directory? how is it a "temp" directory? is it get self destroyed once the process is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, it gets cleaned up by the OS at some point, where that exact point is vague and mysterious but will happen eventually. On Mac OS it's roughly 3 days or when you're running out of space.
This comment has been minimized.
This comment has been minimized.
…it API checks into separate job
b59fd61
to
a768529
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
All other checks are green except for https://github.com/aptos-labs/aptos-core/actions/runs/5336576362/jobs/9671938125?pr=8769. This is a shadow of the past, just kept around by pull_request_target, it will be gone when I land this. Force landing now. |
…it API checks into separate job (#8769)
…it API checks into separate job (#8769)
…it API checks into separate job (aptos-labs#8769)
…it API checks into separate job (aptos-labs#8769)
…it API checks into separate job (#8769)
Description
Currently we run the TS SDK tests against a local testnet built from the same commit. So if you make a PR, it will run a local testnet built from the code in that PR and then run the tests against it. This helps check for compatibility between the SDK and main but that's actually not really necessary. Instead, what matters is confirming that the SDK is compatible with each of the production networks. As such, in this PR I make it that the SDK runs against local testnets built from the production networks (devnet, testnet, mainnet).
To ensure compatibility we run these tests on main and on PRs to check for changes to the SDK that could break compatibility with the production networks.
One nice benefit of this change is the SDK tests no longer have to wait for the whole image build phase, it can just run immediately.
I also switch up
publish_ans_contract.ts
to make the retries actually work in CI. They didn't previously because we weren't deleting the ANS directory properly, so now we use temp directories. The way I do it has to workaround the following issues that GitHub the company probably won't fix any time soon:After removing the TS SDK tests, I spit the remaining stuff into two separate workflows too:
Note: With this new setup, it means that some of the tests as they are now don't work against testnet and mainnet because the fungible asset module is not deployed there yet. To fix this, I made the tests run conditionally depending on the
NETWORK
env var. I also improved the error messages by making the test suite check that the txns are successful.This addresses #8740.
Test Plan
CI. I tested it with
pull_request
instead ofpull_request_target
and the relevant tests (the api compatibility tests) worked.The Rust node API client tests are flaky because of this toolchain issue. It is not related to this PR, I have seen them pass.
I also ran the TS SDK tests locally and they worked.