Skip to content

tec: Fix test suite#2721

Merged
marienfressinaud merged 2 commits intoFreshRSS:masterfrom
flusio:tec/fix-test-suite
Dec 23, 2019
Merged

tec: Fix test suite#2721
marienfressinaud merged 2 commits intoFreshRSS:masterfrom
flusio:tec/fix-test-suite

Conversation

@marienfressinaud
Copy link
Copy Markdown
Member

I realized that unit tests weren't executed on Travis. While working on
this file to enable these tests, I started to think we could simplify
it.

I separated jobs so:

  • PHP linter and tests are only performed on PHP 7.3
  • Translations are tested separatly so they can fail (it was already the
    case but it was hard to understand the way it was done)
  • PHP 5.6 only checks syntax issues
  • the last job checks css, js, etc. (it didn't change)

PHPUnit is not executed on 5.6 because only the version 5 is available
while the latest version is the 8 (https://phpunit.de/supported-versions.html).

I think it's easier to read (each job is more explicit) but I'm not a
Travis expert so maybe there's some room for improvements.

@marienfressinaud
Copy link
Copy Markdown
Member Author

marienfressinaud commented Dec 21, 2019

Ah, my allow_failures system doesn't work anymore! Fixed

@marienfressinaud marienfressinaud force-pushed the tec/fix-test-suite branch 2 times, most recently from ce00960 to 9fd76e5 Compare December 21, 2019 16:05
@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Dec 21, 2019

PHPUnit is not executed on 5.6 because only the version 5 is available

Is there a particular reason this is an issue? The tests are from 2015.

@marienfressinaud
Copy link
Copy Markdown
Member Author

Yes, createMock and (I think) expectException don't exist. Another solution is to use PHPUnit 5 instead of the 8 for PHP 7.3

@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Dec 21, 2019

Another solution is to use PHPUnit 5 instead of the 8 for PHP 7.3

Something along those lines makes more sense to me, because PHP 7.2+ is probably what we run while writing code.

By along those lines I mean that for example PHPUnit 6/PHP 7.0 could be an interesting compromise for using features like createMock. (I can't seem to find when these methods were added.)

@Alkarex Alkarex added this to the 1.16.0 milestone Dec 22, 2019
@marienfressinaud
Copy link
Copy Markdown
Member Author

By along those lines I mean that for example PHPUnit 6/PHP 7.0 could be an interesting compromise for using features like createMock.

createMock is not blocking, we still can switch back to getMock. What bother me is that PHPUnit 5 and 6 are no longer supported. In the mean time, it's only used for tests, it's not like it introduces security issues in FRSS (at least, I don't think so…)

Question is: for which versions do we want to run the test suite? I implicitly answered by "PHP 7.3" because it was an easy solution: the syntax is checked for PHP 5.6 and 7.3, and the FRSS logic doesn't differ between PHP versions. Problems can happen if default values change in PHP functions from a version to another though, or using a non-existing functions. My preference was to use an up-to-date version of PHPUnit, considering that our test coverage is so low that it wouldn't make a big difference to test on several PHP versions. It's a short term opinion but we'll probably stop support of PHP 5.6 and 7.0 in few months/years (official support stopped 1 year ago).
→ Yet, I'm all opened to change to use PHPUnit 5 or 6 or 7, it's really not a big deal and it has its advantages too :)

@marienfressinaud marienfressinaud force-pushed the tec/fix-test-suite branch 2 times, most recently from 4158e02 to 7ec0454 Compare December 22, 2019 15:12
@Frenzie
Copy link
Copy Markdown
Member

Frenzie commented Dec 22, 2019

It doesn't matter too much for now, but in principle I think the tests should be run on multiple PHP versions. It's about behavior, not about syntax.

@marienfressinaud marienfressinaud force-pushed the tec/fix-test-suite branch 4 times, most recently from 9e19cb4 to d89a0f6 Compare December 22, 2019 16:25
@marienfressinaud
Copy link
Copy Markdown
Member Author

I've setup tests for 7.1 and 7.3. I think it's more important to test latest supported versions than oldest. Also it allows to use PHPUnit 7 which is still supported for few weeks.

Copy link
Copy Markdown
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

Lgtm

I realized that unit tests weren't executed on Travis. While working on
this file to enable these tests, I started to think we could simplify
it.

I separated jobs so:

- PHP linter and tests are only performed on PHP 7.3
- Translations are tested separatly so they can fail (it was already the
  case but it was hard to understand the way it was done)
- PHP 5.6 only checks syntax issues
- the last job checks css, js, etc. (it didn't change)

PHPUnit is not executed on 5.6 because only the version 5 is available
while the latest version is the 8 (https://phpunit.de/supported-versions.html).

I think it's easier to read (each job is more explicit) but I'm not a
Travis expert so maybe there's some room for improvements.
The category `_name` regression was introduced in commit b323ed0.

I wasn't able to understand when and why Search tests was wrong.

The rest is about upgrade of PHPUnit.
@marienfressinaud marienfressinaud merged commit 8f5d8af into FreshRSS:master Dec 23, 2019
@marienfressinaud marienfressinaud deleted the tec/fix-test-suite branch December 23, 2019 10:00
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.

3 participants