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

Travis :: Optimize tests #16260

Merged
merged 26 commits into from Dec 29, 2022
Merged

Travis :: Optimize tests #16260

merged 26 commits into from Dec 29, 2022

Conversation

carlosmiei
Copy link
Collaborator

  • added a tests manager that will run everything once a day
  • otherwise, only affected exchanges are tested, unless it's some structural change (inside base, tests, etc)

@@ -1215,7 +1215,7 @@ module.exports = class bybit extends Exchange {
// "nextPageCursor": ""
// },
// "retExtInfo": {},
// "time": 1667533491917
// "time": 1667533491916
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dummy change just to check if bybit's tests are triggered

@carlosmiei carlosmiei closed this Dec 29, 2022
@carlosmiei carlosmiei reopened this Dec 29, 2022
tests-manager.js Outdated
return;
}

const shouldRunEverything = stdout.indexOf ("Exchange") > -1 ||
Copy link
Member

@ttodua ttodua Dec 29, 2022

Choose a reason for hiding this comment

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

Just as a doubt, maybe there are other files beyond these predefined clauses, which might be missing? i.e. some changes in root directory (say keys.json or dependency version in package.sjon or etc.. any file in root directory), or something in dependencies or etc... In the past, I thought that more reliable way could be inverted logic - "...if there is at least one file, that doesn't match derived exchange file path.."
so, iterate through changed files and find if there is even one occurrence where filepath !== './js/${exchangeName}.js && ! filepath.includes('/examples/) then set flag to true.
wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ttodua the thing is, both whitelisting and blacklisting are tricky here, we don't want to run tests, for example if the docs or examples are updated, but we want if run-tests or run-tests-ws are, so I don't think there is a "one size fits all" rule here

Copy link
Member

Choose a reason for hiding this comment

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

but why not blacklist instead of whitelist? i mean, it's easier to check imo : filepath !== './js/${exchangeName}.js && ! filepath.includes('/examples/) && ! filepath.includes('/docs/), because as I said, many things seem to be missing from the whitelisted (not only run-tests but other files that should cause full re-tests/re-build)

@frosty00
Copy link
Member

i've reduced the number of bytes by 75% by rewriting it in bash

Screenshot 2022-12-29 at 11 38 44

@frosty00
Copy link
Member

also will refactor so it just runs once for both pro and rest

@frosty00 frosty00 merged commit c33c441 into ccxt:master Dec 29, 2022
@ttodua
Copy link
Member

ttodua commented Dec 30, 2022

i've reduced the number of bytes by 75% by rewriting it in bash

Screenshot 2022-12-29 at 11 38 44

It's perfect. but for the next cases, please note there is a side-effect - writing in JS has an advantage that most of us will be able to read/edit too, as personally I am not skilled in bash and can't navigate through it :/ I understand, it's in safe hands under you, so no problem, but also in future cases, if the filesize is not critical, maybe we might leave such codes in js.

@carlosmiei carlosmiei deleted the optimize-tests branch January 23, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants