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

A0-3143: Major sync test does not use pre-baked workdir. #1481

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

Marcin-Radecki
Copy link
Contributor

@Marcin-Radecki Marcin-Radecki commented Nov 8, 2023

Description

This PR removes pre-baked workdir file used in major sync test. Motivations are:

  • that workdir was generated on 11.X version. It has happened on the main branch in which there was a change that changed database schema in a way that finalization does not work. This test is supposed to validate whether node is able to sync in major sync mode, ie when gap is greater then 2 sessions, and not major update scenario which it does now
  • workdir contributes to 20% of whole aleph-node size and it bloats git history
  • as it is now, we need to regenerate workdir every time such breaking change occurrs, after this PR is merge we won't have to do it

Also, this PR removes some tech debt which is:

  • removed not used code for python tests, which functionality is covererd by major sync and force reorgs tests
  • added common bash script for running python tests

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Testing

@Marcin-Radecki Marcin-Radecki marked this pull request as ready for review November 8, 2023 14:09
Copy link
Contributor

@kostekIV kostekIV left a comment

Choose a reason for hiding this comment

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

<3

Copy link
Contributor

@woocash2 woocash2 left a comment

Choose a reason for hiding this comment

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

Not having the greatest feeling about test_catch_up and test_multiple_restarts, although test_major_sync is more massive and expects the same result (some node catching up to other nodes) as the removed tests, the removed tests checked slightly different scenarios (for example in test_multiple_restarts the node is restarted many times and expected to catch up each time). While agreeing the value of these tests is low in presence of the major sync test, I still think it is non-zero 😃. Anyways, remove them if you're convinced that it's fine.

@Marcin-Radecki Marcin-Radecki added this pull request to the merge queue Nov 9, 2023
Merged via the queue into main with commit bd7b085 Nov 9, 2023
30 checks passed
@Marcin-Radecki Marcin-Radecki deleted the A0-3141 branch November 9, 2023 07:41
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