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

cache parallel tests log on ci #10989

Merged
merged 10 commits into from Apr 8, 2021

Conversation

hyuraku
Copy link
Contributor

@hyuraku hyuraku commented Apr 1, 2021

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

cache brew tests(parallel tests) log on ci.
Fixes #10507

@MikeMcQuaid
Copy link
Member

Looks good @hyuraku! If you rebase on master the macOS test failures will be gone.

What do we need to look for in the GitHub Actions logs to see if this is working as expected?

@hyuraku
Copy link
Contributor Author

hyuraku commented Apr 2, 2021

@MikeMcQuaid

What do we need to look for in the GitHub Actions logs to see if this is working as expected?

I guess that We have to check the two expect test results.

  1. Cache test log in Cache parallel tests log phase.
    On change order of caching test log · Homebrew/brew@a5c4a3d · GitHub,
    it is not good because cache error happensCache not found for input keys.
  2. Test log shows Using recorded test runtime like a parallel_tests spec

@Bo98
Copy link
Member

Bo98 commented Apr 2, 2021

actions/cache@v1 does not support single file caches. You could create a directory and cache that.

@hyuraku hyuraku force-pushed the paralell_tests_cache_on_ci branch from b4f46b6 to 76a16ca Compare April 3, 2021 09:45
@hyuraku hyuraku force-pushed the paralell_tests_cache_on_ci branch from 76a16ca to 7fbe08e Compare April 3, 2021 10:03
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
uses: actions/cache@v1
with:
path: tests/parallel_runtime_rspec.log
key: ${{ runner.os }}-parallel_runtime_rspec.log
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed: probably want to include the test-flags in the key somehow (because otherwise the three matrix workers will overwrite each other's cache)

@MikeMcQuaid
Copy link
Member

2. Test log shows Using recorded test runtime like a parallel_tests spec

Not seeing any of these yet unfortunately 😭

@Bo98
Copy link
Member

Bo98 commented Apr 5, 2021

The previous cache is probably in a broken state now.

The cache is never overwritten - you have something unique in the key. Use something unique to each test run (like perhaps ${{ github.sha }}), but have restore-keys be more relaxed to allow restoring from previous runs.

@MikeMcQuaid
Copy link
Member

Use something unique to each test run (like perhaps ${{ github.sha }})

That would avoid having any speedup here. It's fine to overwrite the existing values here (as this is effectively what is done locally) it's just not fine to do so across test suites. Arguably, though, that's something that could be handled in brew tests itself to avoid brew tests && brew tests --generic on my local machine having incorrect parallel run logs.

Comment on lines 100 to 106

parallel_rspec_log_path = if ENV["CI"]
"tests/parallel_runtime_rspec.log"
else
"#{HOMEBREW_CACHE}/tests/parallel_runtime_rspec.log"
end

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
parallel_rspec_log_path = if ENV["CI"]
"tests/parallel_runtime_rspec.log"
else
"#{HOMEBREW_CACHE}/tests/parallel_runtime_rspec.log"
end
parallel_rspec_log_name = "parallel_runtime_rspec"
parallel_rspec_log_name = "#{parallel_rspec_log_name}.no_compat" if args.no_compat?
parallel_rspec_log_name = "#{parallel_rspec_log_name}.generic" if args.generic?
parallel_rspec_log_name = "#{parallel_rspec_log_name}.online" if args.online?
parallel_rspec_log_name = "#{parallel_rspec_log_name}.log"
parallel_rspec_log_path = if ENV["CI"]
"tests/#{parallel_rspec_log_name}"
else
"#{HOMEBREW_CACHE}/tests/#{parallel_rspec_log_name}"
end

Comment on lines 211 to 212
key: ${{ runner.os }}-parallel_runtime_rspec.log
restore-keys: ${{ runner.os }}-parallel_runtime_rspec.log
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
key: ${{ runner.os }}-parallel_runtime_rspec.log
restore-keys: ${{ runner.os }}-parallel_runtime_rspec.log
key: ${{ runner.os }}-${{ matrix.test-flags }}-parallel_runtime_rspec

with:
path: tests/parallel_runtime_rspec.log
key: ${{ runner.os }}-parallel_runtime_rspec.log
restore-keys: ${{ runner.os }}-parallel_runtime_rspec.log
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
restore-keys: ${{ runner.os }}-parallel_runtime_rspec.log

@Bo98
Copy link
Member

Bo98 commented Apr 5, 2021

That would avoid having any speedup here.

That's the point of restore keys. So anything is restored via a partial key match but you always save the updated log with a unique key.

Right now, as the key is, the cache will never be updated because the key never changes. So you are storing one log and using it permanently without changes.

(I'm not talking about the platform/flags - I agree they should be separated).

@MikeMcQuaid
Copy link
Member

Right now, as the key is, the cache will never be updated because the key never changes. So you are storing one log and using it permanently without changes.

@Bo98 My understanding was it'd be overwritten each time: https://github.com/Homebrew/brew/pull/10989/checks?check_run_id=2274127135#step:17:3

Do you have a source for the above? Not disagreeing, just want to make sure I fully understand 😁

@Bo98
Copy link
Member

Bo98 commented Apr 6, 2021

That worked because it's the first run under the new key, but look at the previous commit run:

https://github.com/Homebrew/brew/runs/2270091160?check_suite_focus=true#step:17:2

@MikeMcQuaid
Copy link
Member

That worked because it's the first run under the new key, but look at the previous commit run:

https://github.com/Homebrew/brew/runs/2270091160?check_suite_focus=true#step:17:2

Gotcha!

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

@Bo98 make sense to you?

.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@Bo98
Copy link
Member

Bo98 commented Apr 6, 2021

Yep that should work. Will need to run it a couple times to see if the restore works.

@MikeMcQuaid
Copy link
Member

Great work here, thanks again @hyuraku!

@MikeMcQuaid MikeMcQuaid merged commit 5522355 into Homebrew:master Apr 8, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label May 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache parallel_runtime_rspec.log
4 participants