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

StreamSelectLoop: Test suite uses signal constant names in data provider #67

Merged
merged 1 commit into from Feb 15, 2017

Conversation

Projects
None yet
3 participants
@martinschroeder
Contributor

martinschroeder commented Jan 11, 2017

Define constants SIGUSR1, SIGHUP and SIGTERM if they are not defined yet. The signal constants are used in data provider StreamSelectLoopTest::signalProvider() and cause a notice when the pcntl extension is not loaded.

Notice: Use of undefined constant SIGUSR1 - assumed 'SIGUSR1'
@clue

This comment has been minimized.

Member

clue commented Jan 11, 2017

I'm currently undecided so I won't vote yet. Can we really pass values for these missing constants or should we rather make sure the tests are skipped in this case?

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Jan 11, 2017

The tests are skipped when pcntl is not loaded. The problem is that the constants are used in a data provider and PHPUnit will allways call the data provider before the test is skipped from within.

An alternative solution would be to do the checks in the data provider and provide defaults. This way one can get rid of notices without defining constants.

@clue

This comment has been minimized.

Member

clue commented Jan 15, 2017

IMHO it makes more sense to make sure the data provider is only invoked if the extension is actually available. If this is not possible, then I would suggest using signal constant names only and accessing values only through the constant() function.

What do you think?

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Jan 15, 2017

Using constant() still triggers a warning:

Warning: constant(): Couldn't find constant SIGUSR1

I changed the code to do the checks in the data provider. I am not aware of any way to prevent the data provider from being called by PHPUnit. Another solution would be to do the signal testing in another test class and do the check in the setup method but that is too much work IMHO.

@martinschroeder martinschroeder changed the title from Define signal constants when pcntl is not loaded to Provide fallback values for signal constants if they are not defined Jan 16, 2017

@jsor

This comment has been minimized.

Member

jsor commented Feb 3, 2017

From the PHPUnit docs (second blue box):

All data providers are executed before both the call to the setUpBeforeClass static method and the first call to the setUp method. Because of that you can't access any variables you create there from within a data provider. This is required in order for PHPUnit to be able to compute the total number of tests.

https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers.examples.DependencyAndDataProviderCombo.php

Also related: sebastianbergmann/phpunit#836

This current solution looks like a good compromise, so i'm 👍

@jsor

jsor approved these changes Feb 5, 2017

I've recently run into the same while running the test suite on windows, so 👍

@clue

clue approved these changes Feb 6, 2017

Looks okay to me, but consider my comment below anyway 👍

['SIGTERM', SIGTERM],
['SIGUSR1', defined('SIGUSR1') ? SIGUSR1 : 10],
['SIGHUP', defined('SIGHUP') ? SIGHUP : 1],
['SIGTERM', defined('SIGTERM') ? SIGTERM : 15],

This comment has been minimized.

@clue

clue Feb 6, 2017

Member

This looks okay to me. My suggestion would have been to only return the constant names here and then use constant() to access their values within the test instead. This way we would not have to provide any defaults at all and could still skip the tests (possibly via defined()). What do you think about this?

@clue clue added this to the v0.4.3 milestone Feb 8, 2017

@clue

This comment has been minimized.

Member

clue commented Feb 8, 2017

Ping @martinschroeder, what do you think about the above suggestion?

Also, may I ask you to rebase this on the new "0.4" branch so we get this in for the upcoming v0.4.3 release? :shipit:

@clue clue changed the title from Provide fallback values for signal constants if they are not defined to StreamSelectLoop: Test suite provides fallback values for signal constants if they are not defined Feb 8, 2017

@clue clue changed the base branch from master to 0.4 Feb 12, 2017

@clue

This comment has been minimized.

Member

clue commented Feb 12, 2017

I've updated the base branch, can you force-push a rebased version? 👍

@martinschroeder martinschroeder changed the title from StreamSelectLoop: Test suite provides fallback values for signal constants if they are not defined to StreamSelectLoop: Test suite uses signal constant names in data provider Feb 12, 2017

@martinschroeder

This comment has been minimized.

Contributor

martinschroeder commented Feb 12, 2017

I rebased the PR and changed the data provider to use signal constant names as suggested.

@jsor jsor merged commit d913c6f into reactphp:0.4 Feb 15, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jsor jsor referenced this pull request Feb 15, 2017

Merged

Update master from 0.4 #81

@martinschroeder martinschroeder deleted the martinschroeder:signal-constants branch Feb 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment