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

set default value of status input to false + minor readme update #78

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

zmiklank
Copy link
Collaborator

The update_pull_request_status input is now set to false by default.

This change is not backwards compatible.
Users, who wish to have the status in the PR displayed need to specify
this input explicitly in their yaml configuration file.

Reason for this change is, that there are more possible triggers for
TFaGA than Pull Request and Issue Comment and in those this setting is
not applicable when set to true.

Resolves: #65

@zmiklank
Copy link
Collaborator Author

[test]

@zmiklank zmiklank force-pushed the status_input_change branch from 65afb85 to 588afe9 Compare January 18, 2024 13:40
@zmiklank zmiklank marked this pull request as ready for review January 18, 2024 13:40
Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

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

Since you have updated the default settings, you have to update all expect(mocks.request).toHaveBeenCalledTimes(x) checks.

 FAIL  tests/action.test.ts > Integration tests > Minimal
AssertionError: expected "spy" to be called 3 times, but got 1 times
  tests/action.test.ts:221:27
    219| 
    220|     // First call to request PR details, next two calls for setting th…
    221|     expect(mocks.request).toHaveBeenCalledTimes(3);
       |                           ^
    222|     expect(mocks.request).toHaveBeenLastCalledWith(
    223|       'POST /repos/{owner}/{repo}/statuses/{sha}',

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/6]

 FAIL  tests/action.test.ts > Integration tests > Failed test
AssertionError: expected "spy" to be called 3 times, but got 1 times
  tests/action.test.ts:302:27
    300| 
    301|     // First call to request PR details, next two calls for setting th…
    302|     expect(mocks.request).toHaveBeenCalledTimes(3);
       |                           ^
    303|     expect(mocks.request).toHaveBeenLastCalledWith(
    304|       'POST /repos/{owner}/{repo}/statuses/{sha}',

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/6]

 FAIL  tests/action.test.ts > Integration tests > Testing Farm infrastructure error
AssertionError: expected "spy" to be called 3 times, but got 1 times
  tests/action.test.ts:381:27
    379| 
    380|     // First call to request PR details, next two calls for setting th…
    381|     expect(mocks.request).toHaveBeenCalledTimes(3);
       |                           ^
    382|     expect(mocks.request).toHaveBeenLastCalledWith(
    383|       'POST /repos/{owner}/{repo}/statuses/{sha}',

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[3/6]

 FAIL  tests/action.test.ts > Integration tests > Testing Farm request details timeout
AssertionError: expected "spy" to be called 2 times, but got 1 times
  tests/action.test.ts:460:27
    458|     // First call to request PR details, next call for setting the sta…
    459|     // TODO: Check if we set the status to error (it's done in main.ts)
    460|     expect(mocks.request).toHaveBeenCalledTimes(2);
       |                           ^
    461| 
    462|     // Test summary

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[4/6]

 FAIL  tests/action.test.ts > Integration tests > Testing Farm private scope
AssertionError: expected "spy" to be called 3 times, but got 1 times
  tests/action.test.ts:519:27
    517| 
    518|     // First call to request PR details, next two calls for setting th…
    519|     expect(mocks.request).toHaveBeenCalledTimes(3);
       |                           ^
    520|     expect(mocks.request).toHaveBeenLastCalledWith(
    521|       'POST /repos/{owner}/{repo}/statuses/{sha}',

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[5/6]

 FAIL  tests/action.test.ts > Integration tests > Pull Request comment with results
AssertionError: expected "spy" to be called 4 times, but got 2 times
  tests/action.test.ts:593:27
    591| 
    592|     // First call to request PR details, next two calls for setting th…
    593|     expect(mocks.request).toHaveBeenCalledTimes(4);
 Test Files  1 failed | 4 passed (5)
       |                           ^
    594|     expect(mocks.request).toHaveBeenCalledWith(
    595|       'POST /repos/{owner}/{repo}/issues/{issue_number}/comments',

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[6/6]

@zmiklank zmiklank force-pushed the status_input_change branch from 588afe9 to bef5bb3 Compare January 23, 2024 08:36
Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

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

It seems ok; just resolve the conflict.

The sha of latest commit in the PR is obtained from the github metadata
The update_pull_request_status input is now set to false by default.

This change is not backwards compatible.
Users, who wish to have the status in the PR displayed need to specify
this input explicitly in their yaml configuration file.

Reason for this change is, that there are more possible triggers for
TFaGA than Pull Request and Issue Comment and in those this setting is
not applicable when set to true.o

The tests have been adjusted so that only minimal test tests the
configuration when the update_pull_request_status is disabled.
The other tests test non-default behaviour when the PR update is
enabled, as they did test until now.
@zmiklank zmiklank force-pushed the status_input_change branch from bef5bb3 to 5f30148 Compare January 23, 2024 08:51
@zmiklank
Copy link
Collaborator Author

This PR now removes the pr_head_sha input as it is no longer needed. The information provided by this input is now gathered directly from GitHub metadata.

@zmiklank zmiklank merged commit ad6abd7 into sclorg:main Jan 23, 2024
5 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.

Switch default value of update_pull_request_status to false.
2 participants