Skip to content

Only testing doc when only doc is modified#49

Merged
edublancas merged 38 commits into
ploomber:mainfrom
mehtamohit013:doc
Jun 6, 2023
Merged

Only testing doc when only doc is modified#49
edublancas merged 38 commits into
ploomber:mainfrom
mehtamohit013:doc

Conversation

@mehtamohit013
Copy link
Copy Markdown
Contributor

@mehtamohit013 mehtamohit013 commented May 26, 2023

Closes #47

TO DO:

@mehtamohit013 mehtamohit013 changed the title Only testing doc when only doc is modified Only testing doc when only doc is modified - WIP May 26, 2023
@mehtamohit013
Copy link
Copy Markdown
Contributor Author

mehtamohit013 commented May 26, 2023

Hey @edublancas ,
please take a look at the modified.py and please check if it satisfies all the requirements.

I have also added some test that works fine locally, but not with GitHub actions. Seems like I have to use tmp_packages in conftest.py. Any suggestion?

Sample commands:
Context: the current branch has 3 diff: folder doc, another_doc and file a.txt

  1. pkgmt ensure-modified -b main -ex doc -ex another_doc -ex a.txt
    Output: 0

  2. pkgmt ensure-modified -b main -ex another_doc -ex a.txt
    Output: 1

Comment thread src/pkgmt/cli.py Outdated
Comment thread src/pkgmt/cli.py Outdated
Comment thread src/pkgmt/cli.py Outdated
@mehtamohit013 mehtamohit013 marked this pull request as ready for review May 30, 2023 15:44
@mehtamohit013 mehtamohit013 requested a review from edublancas May 30, 2023 15:45
@mehtamohit013
Copy link
Copy Markdown
Contributor Author

@edublancas

open PR with the finished workflow to this repo https://github.com/ploomber/contributing

Also, can you please elaborate more on what you want to add to contributing guidelines?

@mehtamohit013 mehtamohit013 changed the title Only testing doc when only doc is modified - WIP Only testing doc when only doc is modified May 30, 2023
Comment thread src/pkgmt/modified.py
Comment thread src/pkgmt/modified.py Outdated
Comment thread src/pkgmt/modified.py Outdated
@mehtamohit013
Copy link
Copy Markdown
Contributor Author

mehtamohit013 commented May 31, 2023

@edublancas

I have written a template based on the above logic here

Testing:
For testing the logic, I made pkgmt_modi branch with a new GitHub workflow. After that, I made three branches with the following commits on top of pkgmt_modi branch:

  • pkgmt_modi1:
    File Changed: Modified doc/a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

  • pkgmt_modi2:
    File Changed: Modified a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

  • pkgmt_modi3:
    File Changed: Modified a.txt, doc/a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

TLDR: Tests are working as intended

@mehtamohit013 mehtamohit013 requested a review from edublancas May 31, 2023 17:20
Copy link
Copy Markdown
Collaborator

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

Update: nevermind. I realized the branch was hardcoded

am I testing this wrong?

I copied your workflow: https://github.com/ploomber/dummy/blob/main/.github/workflows/doc.yml

added a new file under doc/ ploomber/dummy@32a4fa3

but the output still says: executing all tests

Comment thread src/pkgmt/modified.py Outdated
Comment thread tests/conftest.py
Comment thread tests/conftest.py Outdated
subprocess.run(["git", "commit", "-m", "init-commit-message"])

subprocess.run(["git", "checkout", "-b", "test_modified_doc"])
subprocess.run(["mkdir", "-p", "test_doc1"])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

convert mkdir and touch calls to pathlib.Path since the latter are OS independent

Copy link
Copy Markdown
Contributor Author

@mehtamohit013 mehtamohit013 Jun 1, 2023

Choose a reason for hiding this comment

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

I have converted all the calls to pathlib.Path, but Windows tests are still failing. For more info, please check the tests here

Update: Found the error, working on it

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yeah, some stuff isn't compatible with windows, so no need to add windows and macos to the testing configuration. I suggested changing it to pathlib because at some point we might want to support those OS as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried but it is not working on Windows

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it maybe because of not using ' ' in exclude path like ':^doc' instead of :^doc (due to this error), I will try this approach tomorrow

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope not working
Github Actions

Some links:
Link1
Link2

Comment thread src/pkgmt/modified.py
@@ -0,0 +1,47 @@
import subprocess
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

also create a github workflow with the full logic

it should run the modified command, and if only doc has been modified. it should skip all upcoming steps

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Template

One can use if: steps.check_doc.outcome == 'failure' to determine whether only doc has been modified or not

Comment thread src/pkgmt/modified.py
@mehtamohit013
Copy link
Copy Markdown
Contributor Author

mehtamohit013 commented Jun 2, 2023

@edublancas
Template

With sys.exit
Testing:

  • pkgmt_modi1:
    File Changed: Modified doc/a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

  • pkgmt_modi2:
    File Changed: Modified a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

  • pkgmt_modi3:
    File Changed: Modified a.txt, doc/a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

TLDR: Tests are working as intended

See updated links below

@mehtamohit013 mehtamohit013 requested a review from edublancas June 2, 2023 17:15
@mehtamohit013
Copy link
Copy Markdown
Contributor Author

@edublancas

Template

With windows testing and sys.exit
Testing:

  • pkgmt_modi1:
    File Changed: Modified doc/a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

  • pkgmt_modi2:
    File Changed: Modified a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

  • pkgmt_modi3:
    File Changed: Modified a.txt, doc/a.txt
    Branch Compared: origin/pkgmt_modi
    Output: GitHub Actions

TLDR: Tests are working as intended on ubuntu and macos. Not working for windows

@edublancas
Copy link
Copy Markdown
Collaborator

@mehtamohit013: just to confirm what we discussed earlier today. since windows support is giving too much trouble, let's not worry about it for now

@mehtamohit013
Copy link
Copy Markdown
Contributor Author

@mehtamohit013: just to confirm what we discussed earlier today. since windows support is giving too much trouble, let's not worry about it for now

@edublancas
Ok, then, I believe I have resolved all the issues and the PR is ready to be reviewed and merge

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.

skip tests when only docs/ have been modified

2 participants