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

Separated build step for tests in CI #788

Merged
merged 8 commits into from
Mar 6, 2024
Merged

Conversation

miladz68
Copy link
Contributor

@miladz68 miladz68 commented Feb 29, 2024

Description

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.28%. Comparing base (3230848) to head (fffa043).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
+ Coverage   32.03%   35.28%   +3.25%     
==========================================
  Files         170      171       +1     
  Lines       47462    47481      +19     
==========================================
+ Hits        15203    16754    +1551     
+ Misses      29244    27482    -1762     
- Partials     3015     3245     +230     
Flag Coverage Δ
coreum 32.03% <ø> (ø)
coreum-integration-tests-modules 16.80% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@miladz68 miladz68 changed the title added a separate build step to CI Separated build step for tests in CI Feb 29, 2024
@miladz68 miladz68 marked this pull request as ready for review February 29, 2024 17:22
@miladz68 miladz68 requested a review from a team as a code owner February 29, 2024 17:22
@miladz68 miladz68 requested review from dzmitryhil, ysv and wojtek-coreum and removed request for a team February 29, 2024 17:22
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

What is the difference between the bridge env and coreum, if the bridge build takes 1min, but coreum 3min?

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

I don't know the details of the bridge env, but I don't think they are comparable. we do much more for coreum

  • here we build the coreum binary inside docker (takes 1.5 mins) .
  • we create multiple images (osmosis, cosmos, relayers, and more) (about 1 minute)
  • we download multiple binaries (4 different cored versions, osmosis, hermes, cosmos, and more)
  • we intentionally re-build coreum-builder and znet every time (about 1 min).

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)

a discussion (no related file):
Are we sure this PR resolves the issue we have ?
Shall we discuss it after call ?



.github/workflows/ci.yml line 80 at r2 (raw file):

            codecov: false
          - ci_step: "integration tests faucet"
            build: |

I might be wrong here so don't take it for granted

I think that ideally all our build commands should be exactly same to make sure we don't cache the fastest one. I think it is the easiest way to make it work correctly by design.

If I understand correctly this faucet build step will finish before all other and it is the one which will be cached.

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wojtek-coreum and @ysv)

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

Are we sure this PR resolves the issue we have ?
Shall we discuss it after call ?

we don't have an issue per se. As discussed in the call, we meant to separate the build step to observe how long it takes and then decide what to do. this PR separates the build step and I will argue (later in the call 😂) that there is not much to be done for the build step, unless we change our build in crust significantly.



.github/workflows/ci.yml line 80 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I might be wrong here so don't take it for granted

I think that ideally all our build commands should be exactly same to make sure we don't cache the fastest one. I think it is the easiest way to make it work correctly by design.

If I understand correctly this faucet build step will finish before all other and it is the one which will be cached.

Let's discuss in a call, it will be much faster.

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)


.github/workflows/ci.yml line 80 at r2 (raw file):

Previously, miladz68 (milad) wrote…

Let's discuss in a call, it will be much faster.

discussed on call and decided to try same build command in all steps
Keeping this issue opened

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Decided to check whether the same build for all commands improves the time.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)

@miladz68 miladz68 force-pushed the milad/optimize-cache branch 2 times, most recently from 8c9bc3c to c9f4069 Compare March 4, 2024 08:31
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Done

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wojtek-coreum and @ysv)


.github/workflows/ci.yml line 80 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

discussed on call and decided to try same build command in all steps
Keeping this issue opened

Done.

ysv
ysv previously approved these changes Mar 5, 2024
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)


.github/workflows/ci.yml line 166 at r4 (raw file):

          key: ${{ runner.os }}-cache-smart-contracts-${{ hashFiles('./coreum/**/*.rs') }}
        if: ${{ matrix.wasm-cache }}
      - name: Build step

You have now build step which you don't use and build inside the run

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)


.github/workflows/ci.yml line 166 at r4 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You have now build step which you don't use and build inside the run

Done.

@dzmitryhil dzmitryhil requested a review from ysv March 6, 2024 09:01
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

@miladz68 miladz68 merged commit a2b4319 into master Mar 6, 2024
10 checks passed
@miladz68 miladz68 deleted the milad/optimize-cache branch March 6, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants