Skip to content
This repository has been archived by the owner on Dec 26, 2024. It is now read-only.

test(storage): add random table test #1816

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

DvirYo-starkware
Copy link
Contributor

@DvirYo-starkware DvirYo-starkware commented Mar 18, 2024

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information


This change is Reviewable

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.67%. Comparing base (2511b3a) to head (1c72bfa).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1816   +/-   ##
=======================================
  Coverage   73.67%   73.67%           
=======================================
  Files         127      127           
  Lines       16268    16268           
  Branches    16268    16268           
=======================================
  Hits        11985    11985           
  Misses       2890     2890           
  Partials     1393     1393           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dan-starkware
Copy link
Collaborator

crates/papyrus_storage/src/db/table_types/test_utils.rs line 205 at r1 (raw file):

        debug!("iteration: {iter:?}");
        let wtxn = writer.begin_rw_txn().unwrap();
        let random_op = rng.gen_range(0..3);

Can this depend on the trait somehow?
For example, what happens when we add another operation such as append?

Code quote:

let random_op = rng.gen_range(0..3);

@DvirYo-starkware DvirYo-starkware force-pushed the dvir/random_table_test branch 2 times, most recently from ff6946b to b1cc44b Compare April 14, 2024 11:27
dan-starkware
dan-starkware previously approved these changes Apr 18, 2024
Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Please refrain from using test in test names.

Reviewed 3 of 4 files at r1, 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)

dan-starkware
dan-starkware previously approved these changes Apr 18, 2024
Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)

Copy link
Contributor Author

@DvirYo-starkware DvirYo-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion

a discussion (no related file):

  1. I don't see your comments in reviewable, so I changed what I remember we talked about
  2. There are several changes because of rebase (mainly imports)
  3. the constants were changed to include more options now locally; this test runs ~3 seconds

@DvirYo-starkware DvirYo-starkware force-pushed the dvir/random_table_test branch 14 times, most recently from 4ba9c91 to 5f7a740 Compare April 21, 2024 09:02
@DvirYo-starkware DvirYo-starkware force-pushed the dvir/random_table_test branch 14 times, most recently from c14bda5 to edc7a61 Compare April 24, 2024 07:35
Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r6, 2 of 4 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware)

a discussion (no related file):
@eranreshef-starkware PTAL at the CI part



.github/workflows/ci.yml line 243 at r9 (raw file):

      - uses: dtolnay/rust-toolchain@stable
      - run: DOCKER_BUILDKIT=1 COMPOSE_DOCKER_CLI_BUILD=1 docker build -f papyrus_utilities.Dockerfile .
    

Just a new line

Code quote:

···

crates/papyrus_storage/src/db/table_types/dup_sort_tables_test.rs line 15 at r9 (raw file):

#[test]
fn common_prefix_compare_with_simple_table_random() {
    println!("START");

Please remove

Copy link
Collaborator

@eranreshef-starkware eranreshef-starkware left a comment

Choose a reason for hiding this comment

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

*.yml files look OK

Copy link
Collaborator

@dan-starkware dan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r12, 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)

@DvirYo-starkware DvirYo-starkware added this pull request to the merge queue Apr 30, 2024
Merged via the queue into main with commit 712d257 Apr 30, 2024
17 of 19 checks passed
@DvirYo-starkware DvirYo-starkware deleted the dvir/random_table_test branch April 30, 2024 08:48
ShahakShama pushed a commit that referenced this pull request Apr 30, 2024
* test(storage): add random table test

* CR fix

* Fix ci

* CR fix
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants