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

Add ability to export a WXR to STDOUT #13

Merged
merged 7 commits into from Aug 31, 2017

Conversation

5 participants
@drzraf
Contributor

drzraf commented Aug 18, 2017

Ability to export to stdout.
Sorry for the lack of testing (yet), but having I didn't find a way to use my common wp phar file while overriding the export using the git local copy.
Neither requir'ing in wp-config.php, neither wp --require= made it (hint welcome)

Fixes #9

@miya0001

Hi @drzraf ,

Thanks PR. 😄

Can you add tests?

Show outdated Hide outdated src/Export_Command.php Outdated
Show outdated Hide outdated src/Export_Command.php Outdated
@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 18, 2017

Contributor

Sorry for the lack of testing (yet), but having I didn't find a way to use...

You just run vendor/bin/behat in the git directory and it will use vendor/bin/wp to run your local git copy (this is after doing a composer install).

Contributor

gitlost commented Aug 18, 2017

Sorry for the lack of testing (yet), but having I didn't find a way to use...

You just run vendor/bin/behat in the git directory and it will use vendor/bin/wp to run your local git copy (this is after doing a composer install).

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 21, 2017

Contributor

updated the PR, but vendor/bin/behat throws:

  [RuntimeException]                                                                     
  Exception has been thrown in "beforeSuite" hook, defined in FeatureContext::prepare()  
  $ wp core download --force --path='/tmp/wp-cli-test-core-download-cache'               
  Creating directory '/tmp/wp-cli-test-core-download-cache/'.                            
  Warning: cURL error 28: Resolving timed out after 10508 milliseconds                   
  Error: cURL error 28: Resolving timed out after 10513 milliseconds                     
  cwd:                                                                                   
  exit status: 1

but at least I was able to run the vendor/bin/wp using locally modified code what let me fix further the patch

Contributor

drzraf commented Aug 21, 2017

updated the PR, but vendor/bin/behat throws:

  [RuntimeException]                                                                     
  Exception has been thrown in "beforeSuite" hook, defined in FeatureContext::prepare()  
  $ wp core download --force --path='/tmp/wp-cli-test-core-download-cache'               
  Creating directory '/tmp/wp-cli-test-core-download-cache/'.                            
  Warning: cURL error 28: Resolving timed out after 10508 milliseconds                   
  Error: cURL error 28: Resolving timed out after 10513 milliseconds                     
  cwd:                                                                                   
  exit status: 1

but at least I was able to run the vendor/bin/wp using locally modified code what let me fix further the patch

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 21, 2017

Contributor

updated the PR to avoid cluttering the XML with All done with export. message

Contributor

drzraf commented Aug 21, 2017

updated the PR to avoid cluttering the XML with All done with export. message

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 22, 2017

Contributor

Error: cURL error 28: Resolving timed out after 10513 milliseconds

You seem to have a connectivity issue. Does vendor/bin/wp core download --path=/tmp/test_bin_wp_core_download work for you? Or a standard wp core download --path=/tmp/test_wp_core_download (where wp points to the latest stable)?

Contributor

gitlost commented Aug 22, 2017

Error: cURL error 28: Resolving timed out after 10513 milliseconds

You seem to have a connectivity issue. Does vendor/bin/wp core download --path=/tmp/test_bin_wp_core_download work for you? Or a standard wp core download --path=/tmp/test_wp_core_download (where wp points to the latest stable)?

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 22, 2017

Contributor

Re code, you should remove check_stdout() as it's always getting called in validate_args() and setting $this->stdout. It's not needed anyway as it's just a flag - the mutually exclusive condition should be checked at the top level in __invoke() after the call to validate_args() eg

$stdout = WP_CLI\Utils\get_flag_value( $assoc_args, 'stdout' );
if ( $stdout && WP_CLI\Utils\get_flag_value( $assoc_args, 'dir' ) ) {
	WP_CLI::error( '--stdout and --dir are mutually exclusive.' );
}

(or maybe ...cannot be used together to be more like other messages) and then use $stdout in your conditions.

Contributor

gitlost commented Aug 22, 2017

Re code, you should remove check_stdout() as it's always getting called in validate_args() and setting $this->stdout. It's not needed anyway as it's just a flag - the mutually exclusive condition should be checked at the top level in __invoke() after the call to validate_args() eg

$stdout = WP_CLI\Utils\get_flag_value( $assoc_args, 'stdout' );
if ( $stdout && WP_CLI\Utils\get_flag_value( $assoc_args, 'dir' ) ) {
	WP_CLI::error( '--stdout and --dir are mutually exclusive.' );
}

(or maybe ...cannot be used together to be more like other messages) and then use $stdout in your conditions.

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 22, 2017

Contributor

you were right. I went a bit further. But every tests throw this:

     Given a WP install                 # features/steps/given.php:73
      $ wp core install --url='http://example.com' --title='WP CLI Site' --admin_user='admin' --admin_email='admin@example.com' --admin_password='password1'
      
      PHP Fatal error:  Uncaught Error: Call to undefined function WP_CLI\add_filter() in vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php:1057
      Stack trace:
      #0 vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(983): WP_CLI\Runner->load_wordpress()
      #1 vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php(23): WP_CLI\Runner->start()
      #2 vendor/wp-cli/wp-cli/php/bootstrap.php(75): WP_CLI\Bootstrap\LaunchRunner->process(Object(WP_CLI\Bootstrap\BootstrapState))
      #3 vendor/wp-cli/wp-cli/php/wp-cli.php(23): WP_CLI\bootstrap()
      #4 vendor/wp-cli/wp-cli/php/boot-fs.php(17): include_once('/home/yug/comp/...')
      #5 {main}
        thrown in vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php on line 1057
      cwd: /tmp/wp-cli-test-run-export.feature.439-599c71f91cfc12.96783843/
      exit status: 255

updated the PR

Contributor

drzraf commented Aug 22, 2017

you were right. I went a bit further. But every tests throw this:

     Given a WP install                 # features/steps/given.php:73
      $ wp core install --url='http://example.com' --title='WP CLI Site' --admin_user='admin' --admin_email='admin@example.com' --admin_password='password1'
      
      PHP Fatal error:  Uncaught Error: Call to undefined function WP_CLI\add_filter() in vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php:1057
      Stack trace:
      #0 vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(983): WP_CLI\Runner->load_wordpress()
      #1 vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php(23): WP_CLI\Runner->start()
      #2 vendor/wp-cli/wp-cli/php/bootstrap.php(75): WP_CLI\Bootstrap\LaunchRunner->process(Object(WP_CLI\Bootstrap\BootstrapState))
      #3 vendor/wp-cli/wp-cli/php/wp-cli.php(23): WP_CLI\bootstrap()
      #4 vendor/wp-cli/wp-cli/php/boot-fs.php(17): include_once('/home/yug/comp/...')
      #5 {main}
        thrown in vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php on line 1057
      cwd: /tmp/wp-cli-test-run-export.feature.439-599c71f91cfc12.96783843/
      exit status: 255

updated the PR

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 23, 2017

Contributor

You're getting some very strange errors. What is your development environment?

For some reason WP isn't loading correctly. You could try doing rm -rf /tmp/wp-cli-test-* but until you solve your development environment issues you'll probably hit something else.

Contributor

gitlost commented Aug 23, 2017

You're getting some very strange errors. What is your development environment?

For some reason WP isn't loading correctly. You could try doing rm -rf /tmp/wp-cli-test-* but until you solve your development environment issues you'll probably hit something else.

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 24, 2017

Contributor

Actually got an error yesterday similar to

Warning: cURL error 28: Resolving timed out after 10508 milliseconds

due to github.com being out/slow I think (it proceeded to succeed the second time due to the free retry in Utils\http_request() without verify) so a) the error message should be more helpful and mention the url b) you should only get a retry if it's really a verify issue and c) the scaremongering re your development environment @drzraf I was doing in #13 (comment) is probably unfounded.

Anyway I'll open an issue re a) and b).

Contributor

gitlost commented Aug 24, 2017

Actually got an error yesterday similar to

Warning: cURL error 28: Resolving timed out after 10508 milliseconds

due to github.com being out/slow I think (it proceeded to succeed the second time due to the free retry in Utils\http_request() without verify) so a) the error message should be more helpful and mention the url b) you should only get a retry if it's really a verify issue and c) the scaremongering re your development environment @drzraf I was doing in #13 (comment) is probably unfounded.

Anyway I'll open an issue re a) and b).

@miya0001

This comment has been minimized.

Show comment
Hide comment
@miya0001

miya0001 Aug 24, 2017

Member

I found out that wp db export can export to stdout with the - option like following.

$ wp db export -

There are 2 options.

  • It should support - too.
  • Change the --stdout to -.

Related: wp-cli/db-command#35

Member

miya0001 commented Aug 24, 2017

I found out that wp db export can export to stdout with the - option like following.

$ wp db export -

There are 2 options.

  • It should support - too.
  • Change the --stdout to -.

Related: wp-cli/db-command#35

@miya0001

This comment has been minimized.

Show comment
Hide comment
@miya0001

miya0001 Aug 24, 2017

Member

If it supports - and --stdout, wp db export has to support --stdout too, but it supports only -, I think wp-cli/db-command#35 should be closed.

Member

miya0001 commented Aug 24, 2017

If it supports - and --stdout, wp db export has to support --stdout too, but it supports only -, I think wp-cli/db-command#35 should be closed.

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 25, 2017

Contributor

semantics: option vs positional arguments.
I think the main argument for a db export is "what (exactly) to export" rather than "where to export".
Thus I'd reserve positional argument for some filtering things (with mysqldump in mind to choose db/tables).
In that case, where to generate the output is something optional (and good default may be encountered).
=> For settings the output I favor any option (among -o - or -o <file>, -o stdout or --stdout ... or make it the default)
Side note: IMHO wp db export - is not very good semantics (and quite unusual). But wp db import - seems sane (input data source is the main object of the command)

The same apply for wp [data] export, where to output (directory/file) or how to output (format/...) are not such a great deal (not the main point of the command)
IHMO positional argument, if it were to be introduced for wp export should be reserved to "what to export" rather than how to output the export.
And again, where to output is more an option (-o | --output) than a positional argument.

Contributor

drzraf commented Aug 25, 2017

semantics: option vs positional arguments.
I think the main argument for a db export is "what (exactly) to export" rather than "where to export".
Thus I'd reserve positional argument for some filtering things (with mysqldump in mind to choose db/tables).
In that case, where to generate the output is something optional (and good default may be encountered).
=> For settings the output I favor any option (among -o - or -o <file>, -o stdout or --stdout ... or make it the default)
Side note: IMHO wp db export - is not very good semantics (and quite unusual). But wp db import - seems sane (input data source is the main object of the command)

The same apply for wp [data] export, where to output (directory/file) or how to output (format/...) are not such a great deal (not the main point of the command)
IHMO positional argument, if it were to be introduced for wp export should be reserved to "what to export" rather than how to output the export.
And again, where to output is more an option (-o | --output) than a positional argument.

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 25, 2017

Member

The general convention in the Unix world is that when a positional parameter denotes a file, the - is a shorthand to refer to the corresponding /dev/stdin or /dev/stdout.

Here's the relevant signatures:
wp import [file] - input file as positional parameter => - should be accepted as STDIN
wp export - export file not as positional parameter => - should not be accepted
wp db import [file] - input file as positional parameter => - should be accepted as STDIN
wp db export [file] - export file as positional parameter => - should be accepted as STDOUT

The main difference between wp import/export and wp db import/export is that the former generally produces multiple files, so STDOUT needs to be a special case either way.

Member

schlessera commented Aug 25, 2017

The general convention in the Unix world is that when a positional parameter denotes a file, the - is a shorthand to refer to the corresponding /dev/stdin or /dev/stdout.

Here's the relevant signatures:
wp import [file] - input file as positional parameter => - should be accepted as STDIN
wp export - export file not as positional parameter => - should not be accepted
wp db import [file] - input file as positional parameter => - should be accepted as STDIN
wp db export [file] - export file as positional parameter => - should be accepted as STDOUT

The main difference between wp import/export and wp db import/export is that the former generally produces multiple files, so STDOUT needs to be a special case either way.

@miya0001

This comment has been minimized.

Show comment
Hide comment
@miya0001

miya0001 Aug 25, 2017

Member

Ah, I (probably) understand. 😄

wp export doesn't have any positional arguments, so we need --stdout.
But wp db export has a positional arguments as a file, so - should be accepted.
They are not same case.

Thanks!

I will add example for wp db export - and I should not add --stdout to wp db export.
Is my understanding right?

Member

miya0001 commented Aug 25, 2017

Ah, I (probably) understand. 😄

wp export doesn't have any positional arguments, so we need --stdout.
But wp db export has a positional arguments as a file, so - should be accepted.
They are not same case.

Thanks!

I will add example for wp db export - and I should not add --stdout to wp db export.
Is my understanding right?

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 25, 2017

Member

I'm amenable to adding --stdout to both wp db export and wp export, unless @schlessera has objections. I think having a named flag is more obvious to the end user.

Member

danielbachhuber commented Aug 25, 2017

I'm amenable to adding --stdout to both wp db export and wp export, unless @schlessera has objections. I think having a named flag is more obvious to the end user.

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 25, 2017

Contributor

wp db export - : export file should not be a positional parameter but rather an option. Positional argument should rather be reserved to what to output (like with mysqldump. We don't do mysqldump myfile but rather mysqldump mydatabase > myfile)

The main difference between wp import/export and wp db import/export is that the former generally produces multiple files

(only "export" produces files)
The fact that wp export produces multiples files is AFAICT only the result of a technical and historical limitation related to the memory hog reading/importing XML with PHP5.

I don't think it should be set in stone. On the contrary, the road should be to improve import so that this chunking mechanism may almost disappear in the future. Dumping to stdout by default (and use -o <filename> / --stdout) would be more K.I.S.S.

Moreover, splitting XML <item> may be done externally and more generically using a ~10 lines XSL transformation.

IMHO, if even needed it's up to XML importer to manage heavy XML files.
Importer can perfectly (and should) split the dumps itself before parsing them if it thinks to going to exceed memory_limit.

Contributor

drzraf commented Aug 25, 2017

wp db export - : export file should not be a positional parameter but rather an option. Positional argument should rather be reserved to what to output (like with mysqldump. We don't do mysqldump myfile but rather mysqldump mydatabase > myfile)

The main difference between wp import/export and wp db import/export is that the former generally produces multiple files

(only "export" produces files)
The fact that wp export produces multiples files is AFAICT only the result of a technical and historical limitation related to the memory hog reading/importing XML with PHP5.

I don't think it should be set in stone. On the contrary, the road should be to improve import so that this chunking mechanism may almost disappear in the future. Dumping to stdout by default (and use -o <filename> / --stdout) would be more K.I.S.S.

Moreover, splitting XML <item> may be done externally and more generically using a ~10 lines XSL transformation.

IMHO, if even needed it's up to XML importer to manage heavy XML files.
Importer can perfectly (and should) split the dumps itself before parsing them if it thinks to going to exceed memory_limit.

@miya0001

This comment has been minimized.

Show comment
Hide comment
@miya0001

miya0001 Aug 25, 2017

Member

Thanks @drzraf

wp db export - : export file should not be a positional parameter but rather an option. Positional argument should rather be reserved to what to output (like with mysqldump. We don't do mysqldump myfile but rather mysqldump mydatabase > myfile)

I understand.
But we already have positional argument wp db export [<file>], I guess we should keep it for backward compatibility.
I create a new pull-request to add --stdout to wp db export for now.
wp-cli/db-command#37

I'll keep in my mind your advise, thanks. 👍

Member

miya0001 commented Aug 25, 2017

Thanks @drzraf

wp db export - : export file should not be a positional parameter but rather an option. Positional argument should rather be reserved to what to output (like with mysqldump. We don't do mysqldump myfile but rather mysqldump mydatabase > myfile)

I understand.
But we already have positional argument wp db export [<file>], I guess we should keep it for backward compatibility.
I create a new pull-request to add --stdout to wp db export for now.
wp-cli/db-command#37

I'll keep in my mind your advise, thanks. 👍

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 25, 2017

Contributor

@drzraf did you resolve your issue in #13 (comment) re WP loading incorrectly?

Contributor

gitlost commented Aug 25, 2017

@drzraf did you resolve your issue in #13 (comment) re WP loading incorrectly?

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 25, 2017

Contributor

$ du -sh /tmp/wp-cli*

200M total # that fill-up my /tmp

[edit]
sudo mount -t tmpfs none /tmp/

13 scenarios (1 success, 12 failures)
216 steps (143 success, 61 ignored, 12 failures)
10m17.135s

[edit2]
Stopping Feature from rm/cp'ing each scenario:

13 scenarios (1 success, 12 failures)
216 steps (76 success, 125 ignored, 12 failures)
1m32.258s

Not so bad for a 1/10 of the time

Contributor

drzraf commented Aug 25, 2017

$ du -sh /tmp/wp-cli*

200M total # that fill-up my /tmp

[edit]
sudo mount -t tmpfs none /tmp/

13 scenarios (1 success, 12 failures)
216 steps (143 success, 61 ignored, 12 failures)
10m17.135s

[edit2]
Stopping Feature from rm/cp'ing each scenario:

13 scenarios (1 success, 12 failures)
216 steps (76 success, 125 ignored, 12 failures)
1m32.258s

Not so bad for a 1/10 of the time

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 25, 2017

Contributor

Is that a hard limit? 200M isn't much by today's standards. Most of it is probably in /tmp/wp-cli-test-run-* due to tests failing.

If you're stuck with that limitation, you could try first doing a rm -rf /tmp/wp-cli-* and then running each test one at a time until it passes, cleaning up afterwards, eg

vendor/bin/behat features/export.feature:3
rm -rf /tmp/wp-cli-test-run-*
vendor/bin/behat features/export.feature:12
rm -rf /tmp/wp-cli-test-run-*

Re code, it's still failing because of https://github.com/wp-cli/export-command/pull/13/files#diff-e3d90c1b28e749135782faa062b59c9dR212 which should be removed. Also I don't see the advantage of the variant https://github.com/wp-cli/export-command/pull/13/files#diff-e3d90c1b28e749135782faa062b59c9dR126 as opposed to the suggestion in #13 (comment), nor the need to check for filename_format. It'd also be easier to follow if you didn't squash your commits.

Contributor

gitlost commented Aug 25, 2017

Is that a hard limit? 200M isn't much by today's standards. Most of it is probably in /tmp/wp-cli-test-run-* due to tests failing.

If you're stuck with that limitation, you could try first doing a rm -rf /tmp/wp-cli-* and then running each test one at a time until it passes, cleaning up afterwards, eg

vendor/bin/behat features/export.feature:3
rm -rf /tmp/wp-cli-test-run-*
vendor/bin/behat features/export.feature:12
rm -rf /tmp/wp-cli-test-run-*

Re code, it's still failing because of https://github.com/wp-cli/export-command/pull/13/files#diff-e3d90c1b28e749135782faa062b59c9dR212 which should be removed. Also I don't see the advantage of the variant https://github.com/wp-cli/export-command/pull/13/files#diff-e3d90c1b28e749135782faa062b59c9dR126 as opposed to the suggestion in #13 (comment), nor the need to check for filename_format. It'd also be easier to follow if you didn't squash your commits.

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 25, 2017

Contributor
  • About some of the warning caused by reusing the wp instance + database, see issue: wp-cli/extension-command#34
  • Added a new commit + a testcase.
  • about the check on $assoc_args it's needed in order to catch --stdout --filename_format given that there are no other way that I know to check input value (rather than default value)
    After wp_parse_args() there is no way to know about "real" command-line options anymore.
    Feel free to change/merge to what best fit your needs

best regards

Contributor

drzraf commented Aug 25, 2017

  • About some of the warning caused by reusing the wp instance + database, see issue: wp-cli/extension-command#34
  • Added a new commit + a testcase.
  • about the check on $assoc_args it's needed in order to catch --stdout --filename_format given that there are no other way that I know to check input value (rather than default value)
    After wp_parse_args() there is no way to know about "real" command-line options anymore.
    Feel free to change/merge to what best fit your needs

best regards

@danielbachhuber

Looking pretty good from my perspective. Just a few minor things to finish up.

Show outdated Hide outdated src/Export_Command.php Outdated
Show outdated Hide outdated src/Export_Command.php Outdated
Show outdated Hide outdated src/WP_Export_Stdout_Writer.php Outdated
Show outdated Hide outdated src/WP_Export_Stdout_Writer.php Outdated

@danielbachhuber danielbachhuber changed the title from Stdout export to Add ability to export a WXR to STDOUT Aug 25, 2017

@danielbachhuber danielbachhuber added this to the 1.0.3 milestone Aug 25, 2017

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 26, 2017

Contributor

fixed 3o4 comments

Contributor

drzraf commented Aug 26, 2017

fixed 3o4 comments

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 26, 2017

Member

wp db export - : export file should not be a positional parameter but rather an option. Positional argument should rather be reserved to what to output (like with mysqldump. We don't do mysqldump myfile but rather mysqldump mydatabase > myfile)

I agree with the reasoning, but this is not how the current wp db export command works, and we cannot just randomly introduce breaking changes.

So, yes, we can add the --stdout parameter to wp export as an addition. But we cannot change the positional parameters for wp db export to match something like mysqldump. This will break existing scripts.

Member

schlessera commented Aug 26, 2017

wp db export - : export file should not be a positional parameter but rather an option. Positional argument should rather be reserved to what to output (like with mysqldump. We don't do mysqldump myfile but rather mysqldump mydatabase > myfile)

I agree with the reasoning, but this is not how the current wp db export command works, and we cannot just randomly introduce breaking changes.

So, yes, we can add the --stdout parameter to wp export as an addition. But we cannot change the positional parameters for wp db export to match something like mysqldump. This will break existing scripts.

Tests were added

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 31, 2017

Member

@drzraf ^ Worth taking a look through the changes I made, for your own information.

Member

danielbachhuber commented Aug 31, 2017

@drzraf ^ Worth taking a look through the changes I made, for your own information.

@drzraf

This comment has been minimized.

Show comment
Hide comment
@drzraf

drzraf Aug 31, 2017

Contributor

Indeed I had a look. I hope new behat steps could be made like "Then export file is valid" that would export/import and check identity simplifying new tests?
Thank you for the merge!

Contributor

drzraf commented Aug 31, 2017

Indeed I had a look. I hope new behat steps could be made like "Then export file is valid" that would export/import and check identity simplifying new tests?
Thank you for the merge!

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 31, 2017

Member

I hope new behat steps could be made like "Then export file is valid" that would export/import and check identity simplifying new tests?

Certainly, could. WP-CLI command tests inherit from the main WP-CLI test suite, so it typically makes sense to add new steps when the same test pattern is used in multiple places.

Also, my preference is to use more literal steps so it's more obvious what process is being followed. The more abstraction we add, the higher base knowledge required for a new person to come into the project.

Member

danielbachhuber commented Aug 31, 2017

I hope new behat steps could be made like "Then export file is valid" that would export/import and check identity simplifying new tests?

Certainly, could. WP-CLI command tests inherit from the main WP-CLI test suite, so it typically makes sense to add new steps when the same test pattern is used in multiple places.

Also, my preference is to use more literal steps so it's more obvious what process is being followed. The more abstraction we add, the higher base knowledge required for a new person to come into the project.

@danielbachhuber danielbachhuber merged commit 1b40f23 into wp-cli:master Aug 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment