drop buildkite pipeline

[Ekleog-NEAR]

We have now fully migrated off buildkite, so let’s remove the old pipeline from the repository :)

[Ekleog-NEAR]

Considering we’re no longer using buildkite, I’m removing the check_pytests.py script. It might be possible to port it to github actions, but we’re also looking towards moving the tests from github actions to a Justfile, which would make them easier to run locally.

All in all, I don’t think this script pulls its weight currently. Maybe it will again once our setup will have stabilized. But with today’s movement, I think the risk it mitigates (a python test that would not be run in CI) is not likely enough to warrant the maintenance costs. (Since the beginning of the year, we added a total of 12 pytests, if I count correctly)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2b24bc8) 71.87% compared to head (4714c47) 71.87%.
Report is 17 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #10227    +/-   ##
========================================
  Coverage   71.87%   71.87%            
========================================
  Files         707      707            
  Lines      141791   142220   +429     
  Branches   141791   142220   +429     
========================================
+ Hits       101907   102223   +316     
- Misses      35171    35276   +105     
- Partials     4713     4721     +8     

Flag	Coverage Δ
backward-compatibility	0.08% <ø> (-0.01%)	⬇️
db-migration	0.08% <ø> (-0.01%)	⬇️
genesis-check	1.23% <ø> (-0.01%)	⬇️
integration-tests	36.40% <ø> (+0.22%)	⬆️
linux	71.70% <ø> (-0.04%)	⬇️
linux-nightly	71.65% <ø> (-0.01%)	⬇️
macos	55.05% <ø> (-0.70%)	⬇️
pytests	1.46% <ø> (-0.01%)	⬇️
sanity-checks	1.26% <ø> (-0.01%)	⬇️
unittests	68.16% <ø> (-0.06%)	⬇️
upgradability	0.13% <ø> (-0.01%)	⬇️

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.

[wacban]

Considering we’re no longer using buildkite, I’m removing the check_pytests.py script.

I see why we no longer need to check buildkite but it seems valuable to check that all tests are included in nayduck. Perhaps we can just limit the scope of this script to that job and add exceptions for any scripts that should be run not in nayduck but as separate jobs?

[Ekleog-NEAR]

Hmm… what if we were to split python tests into two folders, one of tests that should all be run by nayduck, and for the like 3 files that are being run by the CI? Would it be hard to do so? (I have basically never touched our pytest infra yet)

[wacban]

Sure that sounds good. @andrei-near is the expert I also have little context on how it actually works.

[nagisa]

I suggest splitting up this PR into two parts – the uncontraversial part that matches the PR title, and the other part can stand its own ground against any scrutiny :)

[Ekleog-NEAR]

While I agree in principle, it’s actually the other way around: check_pytests currently depends on the existence of the buildkite/pipeline.yml file, and thus we cannot drop it without somehow adjusting (or removing) check_pytests 😅

[wacban]

approving to unblock you as it's not that critical, we can continue discussion offline

[nagisa]

adjusting check_pytests

is what I was advocating for. If we do end up landing this removal though we should at least file an issue to follow up on restoring it. But from my reading it should be fairly straightforward to remove the buildkite checks for the time being, here’s how I would suggest to approach it for now:

1. Delete read_pipeline_tests function;
2. Replace missing.difference_update(read_pipeline_tests(BUILDKITE_PIPELINE_FILE)) with a hardcoded (for now; add some TODOs and issues to address it later) list of tests that are covered by GHA.

wdyt?

[andrei-near]

I'm not convinced how this check helps.
On one hand, the list of tests is in this repo and any change should go through a round of reviews before being merged.
On another hand nothing stops someone from disabling a tests by adding a whatever TODO.

[wacban]

The purpose of the check is to make sure that the test author doesn't forget to get it included in nayduck. It's quite easy to miss for both the author and the reviewer.

[Ekleog-NEAR]

I’ve just pushed an updated version of check_scripts.py that does as you say, considering the opinions on the matter :)
Will land soon-ish if no one objects, based on the previous approving reviews-with-comments :)