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

[BridgeImplementationTest] Rewritten unit test to check bridges #980

Merged
merged 8 commits into from
Jan 8, 2019

Conversation

fulmeek
Copy link
Contributor

@fulmeek fulmeek commented Dec 26, 2018

This is a completely rewritten unit test to check whether the bridges are implemented as mentioned in the wiki.

It performs the following tests on each bridge:

  • class name starts with an uppercase character
  • class is an instance of BridgeAbstract
  • all documented constants have the expected types, the required ones are not empty
  • formal inspection of the parameter list
  • class has only public methods that are visible in BridgeAbstract or FeedExpander
  • all documented methods return the expected types
  • the URI is a valid URL and uses HTTPS

However, it does not perform any item checks. Since the generated items heavily depend on the source and the actual parameters, they should be checked directly by the format classes or an intermediate item class to generate a valid feed.

Unfortunately, there are many bridges that do not pass this unit test. They mostly have less clean parameter lists, or they don't use HTTPS even though they usually could. It may take some effort to fix them all. I'm not sure if we should force HTTPS though.

I'm usually not comfortable with completely replacing something that has evolved over the time. What do you think?

@fulmeek
Copy link
Contributor Author

fulmeek commented Dec 26, 2018

I see, Travis already performs this test, which exhibits the problem described above.

Copy link
Contributor

@logmanoriginal logmanoriginal left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Refactoring is a very good idea and it looks like you know how to do this properly. Find below a few comments for improvements.

tests/BridgeImplementationTest.php Outdated Show resolved Hide resolved
tests/BridgeImplementationTest.php Show resolved Hide resolved
tests/BridgeImplementationTest.php Outdated Show resolved Hide resolved
tests/BridgeImplementationTest.php Show resolved Hide resolved
tests/BridgeImplementationTest.php Show resolved Hide resolved
tests/BridgeImplementationTest.php Outdated Show resolved Hide resolved
tests/BridgeImplementationTest.php Outdated Show resolved Hide resolved
tests/BridgeImplementationTest.php Show resolved Hide resolved
tests/BridgeImplementationTest.php Outdated Show resolved Hide resolved
tests/BridgeImplementationTest.php Outdated Show resolved Hide resolved
Copy link
Contributor Author

@fulmeek fulmeek left a comment

Choose a reason for hiding this comment

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

Thanks for your suggestions, they were great.

Note that I omitted the changes testing for empty value strings. I had them your suggested way first, but there are bridges that intentionally use '0' as a valid value. The test would fail then.

logmanoriginal pushed a commit that referenced this pull request Jan 5, 2019
* [DealabsBridge] fixed parameters
* [DemonoidBridge] added parameter context names
* [DevToBridge] fixed parameters
* [ExtremeDownloadBridge] fixed parameters
* [GithubIssueBridge] fixed parameters
* [InstagramBridge] added parameter context names
* [MydealsBridge] fixed parameters
* [OnVaSortirBridge] fixed parameters
* [ThingyverseBridge] fixed parameters
* [HotUKDealsBridge] fixed parameters
* [FeedExpanderExample] added proper URI
* [GQMagazineBridge] fixed parameters and getDomain()
* [MozillaSecurityBridge] fixed filename

References #980
@logmanoriginal
Copy link
Contributor

Thanks for the feedback. You are right, 0 would have set incorrect flags.

I've merged #984. Please rebase this branch to get a successful build.

@fulmeek
Copy link
Contributor Author

fulmeek commented Jan 5, 2019

This should do it :)

@logmanoriginal logmanoriginal merged commit 600f229 into RSS-Bridge:master Jan 8, 2019
@logmanoriginal
Copy link
Contributor

Merged. Thanks again for the PR!

@fulmeek fulmeek deleted the BridgeImplementationTest branch January 9, 2019 22:09
infominer33 pushed a commit to web-work-tools/rss-bridge that referenced this pull request Apr 17, 2020
* [DealabsBridge] fixed parameters
* [DemonoidBridge] added parameter context names
* [DevToBridge] fixed parameters
* [ExtremeDownloadBridge] fixed parameters
* [GithubIssueBridge] fixed parameters
* [InstagramBridge] added parameter context names
* [MydealsBridge] fixed parameters
* [OnVaSortirBridge] fixed parameters
* [ThingyverseBridge] fixed parameters
* [HotUKDealsBridge] fixed parameters
* [FeedExpanderExample] added proper URI
* [GQMagazineBridge] fixed parameters and getDomain()
* [MozillaSecurityBridge] fixed filename

References RSS-Bridge#980
infominer33 pushed a commit to web-work-tools/rss-bridge that referenced this pull request Apr 17, 2020
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.

2 participants