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

BoltSpec expect_upload assumes source file resides inside the module #2905

Closed
reidmv opened this issue Jun 29, 2021 · 7 comments
Closed

BoltSpec expect_upload assumes source file resides inside the module #2905

reidmv opened this issue Jun 29, 2021 · 7 comments
Labels
Bug Bug reports and fixes. Stale Issues and PRs that haven't had activity in 60 days.

Comments

@reidmv
Copy link
Contributor

reidmv commented Jun 29, 2021

Describe the Bug

Currently, the expect_upload BoltSpec function assumes the uploaded file resides inside the module. This limits its usefulness as the following example shows:

https://github.com/puppetlabs/puppetlabs-peadm/blob/main/spec/plans/util/retrieve_and_upload_spec.rb#L17-L27

The above example implements a work-around for mocking the upload_file call as defined here: https://github.com/puppetlabs/puppetlabs-peadm/blob/main/plans/util/retrieve_and_upload.pp#L32

Expected Behavior

When mocking upload_file with an absolute path source argument, the mock should not fail because the file is not present on the filesystem. This is a mock.

Steps to Reproduce

  1. Create a plan that calls upload_file with an absolute source path such as "/tmp/not-a-real-file"
  2. Try and write a spec test for that using expect_upload
@reidmv reidmv added the Bug Bug reports and fixes. label Jun 29, 2021
@MikaelSmith
Copy link
Contributor

Not quite sure how I want to solve this. The problem is the upload_file function expects a file to exist. I started looking at moving the code around checking file existence to the Executor, but that doesn't currently have a direct dependency on Puppet (as a library) and this would add one, which could slow down other operations.

Most straight-forward option seems to be registering another object like inventory/executor with Puppet.lookup and using that. We could then mock it out in BoltSpec. That seems a little convoluted though.

Last one I haven't figured out how to do is directly mock file calls in BoltSpec or override functions rather than the Executor.

@reidmv
Copy link
Contributor Author

reidmv commented Aug 27, 2021

@MikaelSmith if rspec-puppet had a generalized function mock, this would not need to be handled in BoltSpec. A generalized rspec-puppet function mock would actually be the preferred solution to this problem.

@MikaelSmith
Copy link
Contributor

Yeah. I need to test whether the approaches in rodjek/rspec-puppet#684 work for Bolt plans.

@MikaelSmith
Copy link
Contributor

I decided not to try to solve for mocking out the file lookup. It's easy enough to create a temporary file for these tests. Improvements to rspec-puppet would still be welcome, but I think we can make some small improvements here.

@reidmv
Copy link
Contributor Author

reidmv commented Aug 27, 2021

I poked around with repeating the linked workarounds in Bolt last time I looked at this, and was very unsatisfied with the options and results. The existing rspec-puppet means of bumbling through this problem aren't true mocks, they're just workarounds. I wouldn't be keen to simply repeat that hack in BoltSpec. It would be better to provide a real mock, probably using Puppet's internal function loaders somehow. 🤷

MikaelSmith added a commit to MikaelSmith/bolt that referenced this issue Aug 27, 2021
Updates BoltSpec `allow/expect_upload` and `allow/expect_script`
functions to accept an absolute path. The path referred to must still
exist, but that's relatively easy to mock with temporary files.

----

!bug

* **Update BoltSpec functions to allow absolute paths** ([puppetlabs#2905](puppetlabs#2905))

  BoltSpec's `allow_upload`, `expect_upload`, `allow_script`, and
  `expect_script` functions now support passing absolute paths that
  rather than a module reference. The referenced path must refer to a
  real file or upload and script functions will still error.
MikaelSmith added a commit to MikaelSmith/bolt that referenced this issue Aug 27, 2021
Updates BoltSpec `allow/expect_upload` and `allow/expect_script`
functions to accept an absolute path. The path referred to must still
exist, but that's relatively easy to mock with temporary files.

----

!bug

* **Update BoltSpec functions to allow absolute paths** ([puppetlabs#2905](puppetlabs#2905))

  BoltSpec's `allow_upload`, `expect_upload`, `allow_script`, and
  `expect_script` functions now support passing absolute paths that
  rather than a module reference. The referenced path must refer to a
  real file or upload and script functions will still error.
lucywyman added a commit that referenced this issue Sep 10, 2021
(GH-2905) Update BoltSpec functions to allow absolute paths
@github-actions
Copy link

This issue has not had activity for 60 days and will be marked as stale.
If this issue continues to have no activity for 7 days, it will be closed.

@github-actions github-actions bot added the Stale Issues and PRs that haven't had activity in 60 days. label Jul 29, 2022
@github-actions
Copy link

github-actions bot commented Aug 6, 2022

This issue is stale and has been closed. If you believe this is in error,
or would like the Bolt team to reconsider it, please reopen the issue.

@github-actions github-actions bot closed this as completed Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug reports and fixes. Stale Issues and PRs that haven't had activity in 60 days.
Projects
None yet
Development

No branches or pull requests

2 participants