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

chore(infra): share get_absolute_path infra util with snapi test utils #2075

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

ArniStarkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@ArniStarkware ArniStarkware marked this pull request as ready for review November 14, 2024 17:24
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/move_get_absolute_path branch from 63ca81d to 5e61b82 Compare November 17, 2024 06:05
@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/share_get_absolute_path branch from 0141a56 to 42e510a Compare November 17, 2024 06:05
Copy link

Artifacts upload triggered. View details here

Copy link

codecov bot commented Nov 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.21%. Comparing base (e3165c4) to head (a985e92).
Report is 529 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2075       +/-   ##
===========================================
+ Coverage   40.10%   77.21%   +37.11%     
===========================================
  Files          26      386      +360     
  Lines        1895    40386    +38491     
  Branches     1895    40386    +38491     
===========================================
+ Hits          760    31185    +30425     
- Misses       1100     6899     +5799     
- Partials       35     2302     +2267     

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


🚨 Try these New Features:

@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/share_get_absolute_path branch from 42e510a to 104ef4e Compare November 17, 2024 06:44
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/move_get_absolute_path branch from 5e61b82 to 17634f4 Compare November 19, 2024 10:27
@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/share_get_absolute_path branch from 104ef4e to cb4bbcb Compare November 19, 2024 10:27
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/move_get_absolute_path branch from 17634f4 to acaf6a4 Compare November 19, 2024 12:57
@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/share_get_absolute_path branch from cb4bbcb to f154aa3 Compare November 19, 2024 12:58
Copy link

Artifacts upload triggered. View details here

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-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: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)


crates/infra_utils/src/path.rs line 6 at r1 (raw file):

pub static PATH_TO_CARGO_MANIFEST_DIR: LazyLock<Option<PathBuf>> =
    LazyLock::new(|| env::var("CARGO_MANIFEST_DIR").ok().map(|dir| Path::new(&dir).into()));

Please make this a private function, and create a function that returns the cloned path. The benefit of that would be that

  1. users wouldn't have to clone the path whenever they use it
  2. we can later add code that returns this path even when running not through cargo

Code quote:

pub static PATH_TO_CARGO_MANIFEST_DIR: LazyLock<Option<PathBuf>> =
    LazyLock::new(|| env::var("CARGO_MANIFEST_DIR").ok().map(|dir| Path::new(&dir).into()));

@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/move_get_absolute_path branch from acaf6a4 to b179f10 Compare November 19, 2024 15:11
@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/share_get_absolute_path branch from f154aa3 to 4ec2420 Compare November 19, 2024 15:11
Copy link

Artifacts upload triggered. View details here

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.698 ms 35.186 ms 35.761 ms]
change: [+1.4422% +2.9514% +4.7549%] (p = 0.00 < 0.05)
Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
2 (2.00%) high mild
8 (8.00%) high severe

Copy link
Contributor Author

@ArniStarkware ArniStarkware 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: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)


crates/infra_utils/src/path.rs line 6 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please make this a private function, and create a function that returns the cloned path. The benefit of that would be that

  1. users wouldn't have to clone the path whenever they use it
  2. we can later add code that returns this path even when running not through cargo

Done.
We can move functionality from get_absolute_path into a function called: manifest_dir, in another PR.

@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/move_get_absolute_path branch 2 times, most recently from e9d42d0 to eb4e92e Compare November 20, 2024 05:19
@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/share_get_absolute_path branch from 4ec2420 to ca5efcc Compare November 20, 2024 05:19
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/move_get_absolute_path branch from eb4e92e to 5719c03 Compare November 21, 2024 07:20
@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/share_get_absolute_path branch from ca5efcc to 4686fa7 Compare November 21, 2024 07:20
Copy link

Artifacts upload triggered. View details here

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.959 ms 35.457 ms 36.038 ms]
change: [+1.0141% +3.2102% +5.4779%] (p = 0.00 < 0.05)
Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
9 (9.00%) high severe

full_committer_flow performance regressed!
full_committer_flow time: [29.808 ms 29.866 ms 29.948 ms]
change: [+1.0077% +1.2650% +1.5710%] (p = 0.00 < 0.05)
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 14 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

Copy link
Contributor Author

ArniStarkware commented Nov 21, 2024

Merge activity

  • Nov 21, 2:52 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Nov 21, 2:54 AM EST: Graphite rebased this pull request as part of a merge.
  • Nov 21, 3:15 AM EST: A user merged this pull request with Graphite.

@ArniStarkware ArniStarkware changed the base branch from arni/infra_utils/move_get_absolute_path to graphite-base/2075 November 21, 2024 07:52
@ArniStarkware ArniStarkware changed the base branch from graphite-base/2075 to main November 21, 2024 07:52
@ArniStarkware ArniStarkware force-pushed the arni/infra_utils/share_get_absolute_path branch from 4686fa7 to a985e92 Compare November 21, 2024 07:53
Copy link

Artifacts upload triggered. View details here

Copy link

Artifacts upload triggered. View details here

Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [29.991 ms 30.066 ms 30.159 ms]
change: [+1.7203% +2.0320% +2.3621%] (p = 0.00 < 0.05)
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe

@ArniStarkware ArniStarkware merged commit 74b80c4 into main Nov 21, 2024
22 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
@ArniStarkware ArniStarkware deleted the arni/infra_utils/share_get_absolute_path branch November 28, 2024 13:46
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