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.

Sign up for GitHub

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

Jump to bottom

chore: share the test util path_in_resources #2052

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: share the test util path_in_resources #2052

wants to merge 1 commit into from
+33 −34

Conversation

ArniStarkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@ArniStarkware ArniStarkware mentioned this pull request Nov 14, 2024
Merged
@ArniStarkware Graphite App
Copy link
Contributor Author

ArniStarkware commented Nov 14, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ArniStarkware ArniStarkware mentioned this pull request Nov 14, 2024
Merged
@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

Itay-Tsabary-Starkware
Itay-Tsabary-Starkware reviewed Nov 14, 2024
View reviewed changes
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 9 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

crates/starknet_api/src/test_utils.rs line 23 at r1 (raw file):

/// file in the folder "resources".
pub fn path_in_resources<P: AsRef<Path>>(file_path: P) -> PathBuf {
    Path::new(&env::var("CARGO_MANIFEST_DIR").unwrap()).join("resources").join(file_path)

Can this use get_abs_path instead of reading the env variable?

Code quote:

 Path::new(&env::var("CARGO_MANIFEST_DIR").unwrap()).join("resources").join(file_path)

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_get_absolute_path branch 2 times, most recently from 99c0f2f to e127c69 Compare November 14, 2024 10:24
@ArniStarkware ArniStarkware marked this pull request as ready for review November 14, 2024 10:24
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_path_in_resources branch from e011876 to b227fe1 Compare November 14, 2024 10:24
@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

@codecov Codecov
Copy link

codecov bot commented Nov 14, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.23%. Comparing base (e3165c4) to head (c13d674).
Report is 404 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #2052       +/-   ##
===========================================
+ Coverage   40.10%   77.23%   +37.12%     
===========================================
  Files          26      376      +350     
  Lines        1895    39827    +37932     
  Branches     1895    39827    +37932     
===========================================
+ Hits          760    30760    +30000     
- Misses       1100     6778     +5678     
- Partials       35     2289     +2254

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

@ArniStarkware ArniStarkware changed the base branch from arni/test_utils/share_get_absolute_path to graphite-base/2052 November 14, 2024 10:47
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_path_in_resources branch from b227fe1 to c3da22e Compare November 14, 2024 10:47
@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware changed the base branch from graphite-base/2052 to main November 14, 2024 10:48
@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_path_in_resources branch from c3da22e to 3875651 Compare November 14, 2024 10:48
@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

ArniStarkware
ArniStarkware commented Nov 14, 2024
View reviewed changes
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 9 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)

crates/starknet_api/src/test_utils.rs line 23 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Can this use get_abs_path instead of reading the env variable?

Code shared between the two.

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_path_in_resources branch from 3875651 to 83a4abe Compare November 14, 2024 11:14
@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

@ArniStarkware ArniStarkware force-pushed the arni/test_utils/share_path_in_resources branch from 83a4abe to c13d674 Compare November 14, 2024 17:23
@ArniStarkware ArniStarkware mentioned this pull request Nov 14, 2024
Open
This was referenced Nov 14, 2024
Open
Open
@github-actions GitHub Actions
Copy link

Artifacts upload triggered. View details here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware Awaiting requested review from Itay-Tsabary-Starkware

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ArniStarkware @reviewable-StarkWare @Itay-Tsabary-Starkware