Skip to content

remove run-test.sh #3865

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

remove run-test.sh #3865

wants to merge 4 commits into from

Conversation

mmcallister-cll
Copy link
Contributor

Changes

  • removing run-test.sh
  • TODO: add slash back if necessary in Run non-unit, non-integration tests for changed packages failing step from previous PR

Quality Assurance

  • If a new adapter was made, or an existing one was modified so that its environment variables have changed, update the relevant infra-k8s configuration file.
  • If a new adapter was made, or an existing one was modified so that its environment variables have changed, update the relevant adapter-secrets configuration file or update the soak testing blacklist.
  • If a new adapter was made, or a new endpoint was added, update the test-payload.json file with relevant requests.
  • The branch naming follows git flow (feature/x, chore/x, release/x, hotfix/x, fix/x) or is created from Jira.
  • This is related to a maximum of one Jira story or GitHub issue.
  • Types are safe (avoid TypeScript/TSLint features like any and disable, instead use more specific types).
  • All code changes have 100% unit and integration test coverage. If testing is not applicable or too difficult to justify doing, the reasoning should be documented explicitly in the PR.

@mmcallister-cll mmcallister-cll requested a review from a team as a code owner May 14, 2025 23:12
Copy link

changeset-bot bot commented May 14, 2025

🦋 Changeset detected

Latest commit: 8cbd81b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@chainlink/tiingo-adapter Patch
@chainlink/ea-scripts Patch
@chainlink/token-allocation-adapter Patch
@chainlink/apy-finance-adapter Patch
@chainlink/bsol-price-adapter Patch
@chainlink/crypto-volatility-index-adapter Patch
@chainlink/curve-3pool-adapter Patch
@chainlink/defi-dozen-adapter Patch
@chainlink/defi-pulse-adapter Patch
@chainlink/dxdao-adapter Patch
@chainlink/linear-finance-adapter Patch
@chainlink/savax-price-adapter Patch
@chainlink/set-token-index-adapter Patch
@chainlink/synth-index-adapter Patch
@chainlink/vesper-adapter Patch
@chainlink/xsushi-price-adapter Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@mxiao-cll mxiao-cll left a comment

Choose a reason for hiding this comment

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

So this is no longer failing?

@mmcallister-cll
Copy link
Contributor Author

@mxiao-cll need to get the workflow to fail to try another fix directly in it. @dskloetc mentioned the addition of run-test.sh breaks his local test setup.

@dskloetc
Copy link
Contributor

For context, this was added in #3849

I didn't realize the issue was with the workflow rather than manual invocation.

For unit and integration tests we already add /test/unit or /test/integration here and here.
Do we just need to add / in the same way for non-unit-non-integration tests here?

Also, if the problem was that another non-changed package was tested, and that failed, does that mean that that package actually has broken tests? Can we fix those as well?

@mmcallister-cll
Copy link
Contributor Author

@dskloetc summing up some offline discussions:

I might be wrong, but I understood the problem as related to changed packages over-inclusion based on the yarn test glob and their package dependencies not being built, causing the failing workflow run. Specifically apy-finance-test and token-allocation-test EAs that are not dependent on tiingo, but their v2 framework counterparts are, and they're listed in the changed packages list (apy-finance, token-allocation). Here's the failed workflow https://github.com/smartcontractkit/external-adapters-js/actions/runs/14936750420/job/41967035723

Cannot find module '@chainlink/token-allocation-test-adapter' from '/home/runner/work/external-adapters-js/external-adapters-js/packages/composites/apy-finance-test/src/config'

I think this below comment is correct

Do we just need to add / in the same way for non-unit-non-integration tests here?

I think this below comment is not applicable due to the above mentioned dependency build issue

Also, if the problem was that another non-changed package was tested, and that failed, does that mean that that package actually has broken tests? Can we fix those as well?

@dskloetc
Copy link
Contributor

and their package dependencies not being built

Ah, thanks for clarifying.

@dskloetc
Copy link
Contributor

I think this below comment is correct

Do we just need to add / in the same way for non-unit-non-integration tests here?

Do you plan to do that in this PR?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants