tezos-checker / checker

An in-development "robocoin" system for the Tezos blockchain
25 stars 16 forks source link

Test both collateral configurations on CI #303

Closed gkaracha closed 2 years ago

gkaracha commented 2 years ago

Prerequisite for #213.

github-actions[bot] commented 2 years ago
Gas costs: No change. Entrypoint sizes d6ea806e3cf90009c45af51e2dc5a1595fc81d27 4263df3b3bfc4746cda3ed4d2320dcca9fb5c64d Diff
touch 56749 56556 -193
Test coverage d6ea806e3cf90009c45af51e2dc5a1595fc81d27 4263df3b3bfc4746cda3ed4d2320dcca9fb5c64d Diff
price.ml 76.92 100 23.08
TOTAL 92.47 92.57 0.09999999999999432
gkaracha commented 2 years ago

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

gkaracha commented 2 years ago

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

dorranh commented 2 years ago

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

dorranh commented 2 years ago

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 commented 2 years ago

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.

dorranh commented 2 years ago

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.

github-actions[bot] commented 2 years ago
Gas costs: No change. Entrypoint sizes cf1f91319100957147b72112f213cb871f7c7356 adc6c48382969e7478a648c83e0603ef4800cf36 Diff
touch 56749 56556 -193
Test coverage cf1f91319100957147b72112f213cb871f7c7356 adc6c48382969e7478a648c83e0603ef4800cf36 Diff
price.ml 76.92 100 23.08
TOTAL 92.47 92.57 0.09999999999999432
gkaracha commented 2 years ago

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? :thinking: 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 commented 2 years ago

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.