Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

(BOLT-1403) Make bolt-server specs less task-specific #1059

Merged

Conversation

caseywilliams
Copy link
Contributor

Refactors transport_app_spec tests in a way that should allow for easier
additions of other actions (like run_command, upload_file, run_script)
to the bolt server API alongside run_task.

app_integration_spec tests remain the same, except for a renamed helper
method.

# Since this will only be on one node we can just return the first result
result = scrub_stack_trace(results.first.status_hash)
[200, result.to_json]
@executor.run_task(target, task, parameters)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the HTTP response details to the caller here seemed like a simple way to make run_task more focused on just the task operations, but open to changing this

}

post path, JSON.generate(body), 'CONTENT_TYPE' => 'text/json'
context 'when raising errors' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved these app-wide test cases further up, above the transport-specific and action-specific tests. I also removed the task body content passed to this 404 test case, since they should never reach validation anyway .

end
end

describe 'transport routes' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal with moving these test cases to a "transport routes" section was to unit test the /ssh/:action and /winrm/:action logic without making the test content specific to one action. These tests no longer use the :ssh or :winrm tags because the functions they call have been mocked. FWIW, there are actual echo task executions for ssh and winrm further down, but if we'd rather actually run these in the docker/vagrant environment, I can take this back to the drawing board.

Copy link
Contributor

@mcdonaldseanp mcdonaldseanp Jul 3, 2019

Choose a reason for hiding this comment

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

Since there are executions of actual ssh/winrm further down I think there's a low level of risk in your approach here. 👍

}
}
}
describe 'action endpoint' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intent is to use this section for testing of other endpoints like run_command, run_script, or upload_file.

@caseywilliams caseywilliams force-pushed the BOLT-1403/spec-refactor branch from bd7a9a9 to be0b943 Compare June 27, 2019 19:33
@caseywilliams
Copy link
Contributor Author

😟 I'm not sure I understand why there's a connection failure in appveyor -- the same test seems to be passing for me locally with vagrant

Copy link
Contributor

@speedofdark speedofdark left a comment

Choose a reason for hiding this comment

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

Nicely done. LGTM.

@puppetcla
Copy link

CLA signed by all contributors.

Refactors transport_app_spec tests in a way that should allow for easier
additions of other actions (like run_command, upload_file, run_script)
to the bolt server API alongside run_task.

app_integration_spec tests remain the same, except for a renamed helper
method.
@caseywilliams caseywilliams force-pushed the BOLT-1403/spec-refactor branch from be0b943 to f3ba7a7 Compare June 28, 2019 17:28
@caseywilliams
Copy link
Contributor Author

Hmm, now Appveyor is passing, but Travis is failing on docs generation -- I'm not sure what's up, since it was passing before

@donoghuc
Copy link
Contributor

donoghuc commented Jun 28, 2019

Uhh, my best guess is the newest yard gem release changes how puppet-strings works? Put up #1061 but I want to verify that we actually want it. Sorry!

Update: We went with pinning yard #1062 until we fix in puppet-strings puppetlabs/puppet-strings#205 so you should be good to go.

@geoffnichols geoffnichols requested a review from smcclellan June 28, 2019 22:34
@mcdonaldseanp mcdonaldseanp merged commit ad65a54 into puppetlabs:master Jul 3, 2019
@smcclellan smcclellan removed their request for review July 3, 2019 17:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants