Skip to content

Conversation

@dream-encode
Copy link
Contributor

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Just one suggestion to use a data provider for the new test method.

/**
* @ticket 36308
*/
public function test_get_attached_file_with_windows_paths() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using a data provider for the file, expected values, and possibly the message value, will ensure that both assertions run even if one happens to fail, and makes it easier to add more tests for this in future if needed.

Copy link
Member

@johnbillion johnbillion left a comment

Choose a reason for hiding this comment

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

This looks good and there's a good amount of test coverage, but I'd also like to see the tests in Tests_Post_GetAttachedFile use a data provider 👍

@costdev
Copy link
Contributor

costdev commented Aug 11, 2022

This looks good and there's a good amount of test coverage, but I'd also like to see the tests in Tests_Post_GetAttachedFile use a data provider 👍

Hi @johnbillion, see PR 3049 which is a refresh of this PR that uses a data provider in Test_Post_GetAttachedFile

@johnbillion
Copy link
Member

Closing in favour of #3049 👍

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.

4 participants