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

Test for various cli parameter invocations #147

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

mbrsamsung
Copy link
Contributor

No description provided.

@mbrsamsung mbrsamsung marked this pull request as draft January 9, 2025 11:24
@mbrsamsung mbrsamsung force-pushed the mbr/issue-128-unit-tests branch 3 times, most recently from 7d17837 to ca4eea1 Compare January 9, 2025 13:37
@mbrsamsung mbrsamsung marked this pull request as ready for review January 9, 2025 14:06
@coveralls
Copy link

coveralls commented Jan 9, 2025

Pull Request Test Coverage Report for Build 13197802809

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.4%) to 64.685%

Totals Coverage Status
Change from base Build 13180722999: 1.4%
Covered Lines: 1179
Relevant Lines: 1870

💛 - Coveralls

@mbrsamsung mbrsamsung force-pushed the mbr/issue-128-unit-tests branch 3 times, most recently from 57a39f6 to fb7fc54 Compare January 10, 2025 10:17
@mbrsamsung mbrsamsung changed the title feat(tests): test for various cli parameter invocations Test for various cli parameter invocations Jan 14, 2025
@mbrsamsung mbrsamsung force-pushed the mbr/issue-128-unit-tests branch from fb7fc54 to 2e54b0c Compare January 15, 2025 14:26
Copy link
Collaborator

@safl safl left a comment

Choose a reason for hiding this comment

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

A couple of questions inline.

@mbrsamsung mbrsamsung force-pushed the mbr/issue-128-unit-tests branch 15 times, most recently from 76710d6 to 6e57aef Compare January 20, 2025 11:49
Copy link
Collaborator

@safl safl left a comment

Choose a reason for hiding this comment

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

A couple of inline questions

@safl safl force-pushed the main branch 8 times, most recently from d0c8bd7 to 2a4d5f0 Compare January 30, 2025 12:54
@mbrsamsung mbrsamsung force-pushed the mbr/issue-128-unit-tests branch 2 times, most recently from ef74a57 to d6d295c Compare February 3, 2025 08:07
Copy link
Collaborator

@safl safl left a comment

Choose a reason for hiding this comment

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

A couple of comments/questions inline.

@mbrsamsung mbrsamsung force-pushed the mbr/issue-128-unit-tests branch 2 times, most recently from c151e9f to 01f9b4b Compare February 7, 2025 10:00
Copy link
Collaborator

@safl safl left a comment

Choose a reason for hiding this comment

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

A couple of things inline.

@@ -137,6 +138,8 @@ jobs:
cijoe --example ${{ matrix.usage_example }}

- name: Run it!
env:
GHA_RUNNER_TOKEN: "BFGC6XRGGM7PUVLC5NYXARLHQ6FI6"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this token added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There where no other clear way, workflows that runs in a pull request and which are not initiated from a user of the project will not have the secrets included in the run. This will leave the build always failed in that use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! We are in the other PR. This was meant for GHA example. I guess the splitting up of the commits into PR's did result in this being included erroneously

@@ -129,6 +129,7 @@ jobs:
- name: Build and install cijoe from source
run: |
pipx uninstall cijoe
pipx install build
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be needed. make deps on the next line does pipx-install?

@mbrsamsung mbrsamsung force-pushed the mbr/issue-128-unit-tests branch from 01f9b4b to 1bbe034 Compare February 7, 2025 10:18
Copy link
Collaborator

@safl safl left a comment

Choose a reason for hiding this comment

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

lgtm.

@safl safl merged commit e3668b2 into refenv:main Feb 7, 2025
20 checks passed
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.

3 participants