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

Move basic.py to where ci-cd will pick it up from #610

Merged
merged 31 commits into from
Dec 2, 2022

Conversation

pouneh
Copy link
Contributor

@pouneh pouneh commented Nov 30, 2022

Motivation

We want to ensure we do quality checks on ci by running integration tests

In this PR

The ci-cd pipeline currently uses a sparse test. Moving the most up-to-date integ test to where ci reads the tests it uses.
Addresses PT#183925740

Test Plan

You can run this test by navigating to the directory and running poetry run python3 basic.py
Mostly, testing this change is seeing the test run successfully on CI as part of this PR

@pouneh pouneh marked this pull request as draft November 30, 2022 19:40
@pouneh pouneh force-pushed the pouneh/integ-test/183925740 branch from aed17df to f2c540f Compare November 30, 2022 20:04
@cxloe
Copy link
Contributor

cxloe commented Nov 30, 2022

maybe this should be a symlink instead. we could have the integ-tests folder in python-library, then symlink it to the CI dir.

i wonder if this approach would work?

@cxloe cxloe self-requested a review November 30, 2022 20:23
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2022

Codecov Report

Base: 59.28% // Head: 59.29% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e69eaea) compared to base (ad940ae).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #610   +/-   ##
========================================
  Coverage    59.28%   59.29%           
========================================
  Files           82       82           
  Lines        11503    11503           
  Branches      1858     1858           
========================================
+ Hits          6820     6821    +1     
  Misses        3149     3149           
+ Partials      1534     1533    -1     
Impacted Files Coverage Δ
full-service/src/db/wallet_db.rs 24.24% <0.00%> (+0.75%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@brucek
Copy link
Contributor

brucek commented Dec 1, 2022

You can run this test by navigating to the directory and running poetry python3 basic.py

2 Q's for you here:

  1. I seem to need to run poetry run python3 basic.py, am I missing something, or is the description here?
  2. Is there a sample config file somewhere? I also can't seem to run locally without one (I can add my own mnemonics if needed)

Copy link
Contributor

@jgreat jgreat left a comment

Choose a reason for hiding this comment

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

LGTM. Good catch on the docs, not sure how I forgot to put the poetry install step in the README.

@pouneh
Copy link
Contributor Author

pouneh commented Dec 1, 2022

  1. I seem to need to run poetry run python3 basic.py, am I missing something, or is the description here?

    1. Is there a sample config file somewhere? I also can't seem to run locally without one (I can add my own mnemonics if needed)
  1. Yea that is what you need to run, I'm going to update the readme
  2. I'm adding in the sample config

@pouneh
Copy link
Contributor Author

pouneh commented Dec 1, 2022

mm I'm not sure how to do the symlinking in a good way .. like who creates the symlink/maintains it?

@jgreat
Copy link
Contributor

jgreat commented Dec 1, 2022

maybe this should be a symlink instead. we could have the integ-tests folder in python-library, then symlink it to the CI dir.

i wonder if this approach would work?

I think having the integration tests in the test folder makes sense here. Its not a library.

For ease of use and only having to maintain a single set of dependencies, we can have multiple scripts/tests in this fs-integration dir.

Also breaking it up into other directories or renaming it under the test dir is fine.

I kinda think that if we want tests that are integrated with the main repo structure, we should probably write them as rust utilities.

@cxloe
Copy link
Contributor

cxloe commented Dec 1, 2022

mm I'm not sure how to do the symlinking in a good way .. like who creates the symlink/maintains it?

a symlink is just a file that points to another file.. there's not anything to maintain?

@pouneh pouneh force-pushed the pouneh/integ-test/183925740 branch from e4bf33c to 73b67a6 Compare December 1, 2022 20:59
@pouneh
Copy link
Contributor Author

pouneh commented Dec 1, 2022

Do people have to set up that symlink manually on their machine, if not we're checking it in? Can we even checkin a symlink? If we do, will we hit snags when people clone this across different machines? There's just too many questions I don't know how to answer .. and I think fidling til I find out the answer isn't ... really what I want to do with this PR :(

@pouneh pouneh closed this Dec 1, 2022
@pouneh pouneh reopened this Dec 1, 2022
@cxloe
Copy link
Contributor

cxloe commented Dec 1, 2022

Do people have to set up that symlink manually on their machine, if not we're checking it in? Can we even checkin a symlink? If we do, will we hit snags when people clone this across different machines? There's just too many questions I don't know how to answer .. and I think fidling til I find out the answer isn't ... really what I want to do with this PR :(

You can check in a symlink. It should work as long as it's not pointing all the way from a machine specific dir.. so you'd just want to move back a few directories instead of doing something like ln -s /home/user/workspace/some-repo/file .

@pouneh pouneh marked this pull request as ready for review December 2, 2022 01:46
@pouneh pouneh merged commit 590892e into develop Dec 2, 2022
@pouneh pouneh deleted the pouneh/integ-test/183925740 branch December 2, 2022 01:57
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.

5 participants