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

Test both collateral configurations on CI #303

Merged
merged 2 commits into from
Nov 26, 2021

Conversation

gkaracha
Copy link
Contributor

@gkaracha gkaracha commented Nov 24, 2021

Prerequisite for #213.

@gkaracha gkaracha requested a review from dorranh November 24, 2021 15:22
@github-actions
Copy link

github-actions bot commented Nov 24, 2021

Gas costs: No change.

Entrypoint sizes d6ea806 4263df3 Diff
touch 56749 56556 -193
Test coverage d6ea806 4263df3 Diff
price.ml 76.92 100 23.08
TOTAL 92.47 92.57 0.09999999999999432

@gkaracha gkaracha marked this pull request as draft November 24, 2021 15:41
@gkaracha
Copy link
Contributor Author

This seems to be breaking the message bot; reverting back to a draft until we resolve this.

@gkaracha gkaracha marked this pull request as ready for review November 24, 2021 15:53
@gkaracha
Copy link
Contributor Author

Undrafting; the bot seems to be working after all. I guess writing the output twice is problematic in general though 🤔 (and perhaps a little flaky; statistics can be different for different configurations)

@dorranh
Copy link
Contributor

dorranh commented Nov 24, 2021

Ah now that is an unforeseen consequence - perhaps we should start plotting histograms instead 😅

@dorranh
Copy link
Contributor

dorranh commented Nov 24, 2021

In all seriousness, I think that with the current approach we are actually overwriting the coverage, gas cost, etc. artifacts. We might want to split them out per workflow (e.g. with a different suffix) and update the comment bot accordingly.

@gkaracha
Copy link
Contributor Author

Yeah, true. My initial thought was to only allow writing gas costs if ${{ matrix.configuration_file }} == "checker-tez.yaml" but I agree with you. It would be nice to be able to catch regressions for either deployment. Let me turn this again into a draft until this is implemented correctly.

@gkaracha gkaracha marked this pull request as draft November 25, 2021 10:41
@dorranh
Copy link
Contributor

dorranh commented Nov 25, 2021

My initial thought was to only allow writing gas costs if ${{ matrix.configuration_file }} == "checker-tez.yaml"

You know, this might actually be a nicer immediate solution. That way we can go ahead and start testing both and then add support for metrics for FA2 checker in a separate PR.

@gkaracha gkaracha force-pushed the gkaracha/test-both-collateral-configs branch from 4263df3 to adc6c48 Compare November 25, 2021 15:22
@github-actions
Copy link

github-actions bot commented Nov 25, 2021

Gas costs: No change.

Entrypoint sizes cf1f913 adc6c48 Diff
touch 56749 56556 -193
Test coverage cf1f913 adc6c48 Diff
price.ml 76.92 100 23.08
TOTAL 92.47 92.57 0.09999999999999432

@gkaracha
Copy link
Contributor Author

gkaracha commented Nov 25, 2021

My initial thought was to only allow writing gas costs if ${{ matrix.configuration_file }} == "checker-tez.yaml"

You know, this might actually be a nicer immediate solution. That way we can go ahead and start testing both and then add support for metrics for FA2 checker in a separate PR.

More incrementally, eh? 🤔 That sounds good to me; at least we can already safely know that both collateral configurations work, even if we don't get extra gas info about the FA2 one (gas costs can still not go overboard though, because if that happens I expect the corresponding e2e test would fail, so that's another safety net). Let's see how adc6c48 does

@gkaracha gkaracha marked this pull request as ready for review November 25, 2021 16:53
@gkaracha
Copy link
Contributor Author

I'll go ahead and merge this one so that both collateral configurations are always tested and we can always add support for metrics for collateral=FA2 checker in a separate PR; currently the artifacts stored only capture gas costs etc. for collateral=TEZ.

@gkaracha gkaracha merged commit 30bba1a into master Nov 26, 2021
@gkaracha gkaracha deleted the gkaracha/test-both-collateral-configs branch November 26, 2021 12:47
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.

2 participants