Skip to content
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

Removed Internet dependency from test e_library_managerTest::testDetectionVersionConsistency() #4561

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Deltik
Copy link
Member

@Deltik Deltik commented Sep 10, 2021

Fixes: #4560

Motivation and Context

e_library_managerTest::testDetectionVersionConsistency() is pointlessly downloading resources from the Internet, so it was really only testing that the CDN was accessible. This is not what a unit test should be doing.

Description

Changed the behavior of e_library_managerTest::testDetectionVersionConsistency() so that it checks if the CDN library version is the same as the local library version without connecting to the Internet

How Has This Been Tested?

By fixing the existing test so that it actually tests something useful

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist

Removed Internet dependency for test
`e_library_managerTest::testDetectionVersionConsistency()`

And now the test is actually useful because it can now detect the
version mismatch between the local copy and the CDN link

Fixes: e107inc#4560
@Deltik Deltik requested a review from CaMer0n September 10, 2021 15:22
@Deltik Deltik changed the title Removed Internet dependency for test e_library_managerTest::testDetectionVersionConsistency() Removed Internet dependency from test e_library_managerTest::testDetectionVersionConsistency() Sep 10, 2021
@CaMer0n
Copy link
Member

CaMer0n commented Sep 10, 2021

Changed the behavior of e_library_managerTest::testDetectionVersionConsistency() so that it checks if the CDN library version is the same as the local library version without connecting to the Internet

The point of the test was to check that the hard-coded version number located in the library class matched the version number detected and written in the CDN file itself. Hard-coding the version like this improved page-load performance dramatically.

It was never about testing local versions of the library against CDN versions of the same library. Perhaps a separate test should be added for this.

@Deltik
Copy link
Member Author

Deltik commented Sep 10, 2021

The point of the test was to check that the hard-coded version number located in the library class matched the version number detected and written in the CDN file itself. Hard-coding the version like this improved page-load performance dramatically.

Can we mock the Internet part by loading the local files? It should be out of our scope to check that the CDN is serving the right content, as that behavior is not within our control.

It was never about testing local versions of the library against CDN versions of the same library. Perhaps a separate test should be added for this.

The name of the method testDetectionVersionConsistency() suggested to me that the intention was to compare the CDN version with the local version. Maybe this test should become what the name looks like and then a new test be written to check that the CDN URL includes the library's version number.

@Deltik
Copy link
Member Author

Deltik commented Feb 13, 2023

@CaMer0n, we will continue to have this test failure due to the dependency on the Internet while running "unit" tests:

---------
1) e_library_managerTest: Detection version consistency
 Test  tests/unit/e_library_managerTest.php:testDetectionVersionConsistency
No looked-up version in core_library:config() -- cdn.fontawesome
Failed asserting that false is true.
#1  /__w/e107/e107/e107_tests/tests/unit/e_library_managerTest.php:84
#2  /__w/e107/e107/e107_tests/vendor/bin/codecept:115

What changes do you need for approval of this pull request?

@codeclimate
Copy link

codeclimate bot commented Apr 13, 2023

Code Climate has analyzed commit dc634d3 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 20.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 34.9% (0.3% change).

View more on Code Climate.

@Deltik Deltik force-pushed the master branch 2 times, most recently from 9119ebf to 2a0ee3f Compare October 28, 2023 17:52
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.

e_library_managerTest::testDetectionVersionConsistency() depends on the Internet
2 participants