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

Refactor unit tests to use mocks #41

Merged
merged 3 commits into from
Apr 5, 2022
Merged

Refactor unit tests to use mocks #41

merged 3 commits into from
Apr 5, 2022

Conversation

zharinov
Copy link
Contributor

@zharinov zharinov commented Feb 21, 2022

  • Rewrite current tests as close to original as possible
  • Increase coverage

import * as _core from '@actions/core'
import * as _exec from '@actions/exec'
import * as _io from '@actions/io'
import * as _ioUtil from '@actions/io/lib/io-util'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note on io-util usage: it's hard to just mock fs module, since it crashes @actions/io module (here). However, it seems to be a good idea to replace fs usage to @actions/io/lib/io-util, as it gives simplicity of synchronous functions while remaining asynchronous via promises usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPD: instead of poorly documented @actions/io/lib/io-util I've created special fs.ts file that re-exports functions that are used in the action.

@zharinov zharinov marked this pull request as ready for review March 20, 2022 11:18
@zharinov
Copy link
Contributor Author

Would you like to ship some parts of this PR separately, or you're okay with this one?

@DeLaGuardo
Copy link
Owner

Sorry, I was busy with other tasks. From the first look, it is amazing. I think tomorrow I will have some time to check it a bit more. Thanks for your contribution!

@zharinov
Copy link
Contributor Author

Great, thank you 👍

Copy link
Owner

@DeLaGuardo DeLaGuardo left a comment

Choose a reason for hiding this comment

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

LGTM

@DeLaGuardo DeLaGuardo merged commit c54d98d into DeLaGuardo:master Apr 5, 2022
@DeLaGuardo
Copy link
Owner

Thanks again for contribution)

@DeLaGuardo
Copy link
Owner

btw,
image
amazing!

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.

2 participants