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

2015 CPAN PR Challenge - PR #3 - Test HTML Parsing #6

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
1 participant
@bambams
Contributor

bambams commented Jan 30, 2015

This pull request introduces a new test program that demonstrates the flaws in the regular expression used to parse the HTML. I am new to writing unit tests, test-driven development, and writing tests in Perl too. This is just a contrived program to show the alleged flaws. I tried to test it by hard-coding a "correct" transformation of the input HTML and verifying that the tests all pass. Hopefully that is still true after rebasing.

The test program uses File::Temp to log diagnostic information that would be noisy on the terminal. It might be helpful to understand why the tests are failing. Additionally, it might be useful to identify bugs in the tests themselves once work begins on replacing the regular expression with a proper parser.

I originally wanted to submit another branch that actually fixes these tests by using HTML::Parser and reimplmenting both of HTML::FillInForms and HTML::StickyQuery. I ran out of time and motivation this month so officially that patch won't make it into the challenge, but I'll still do my best to come up with something when time and motivation permit unless you beat me to it. Make sure you don't release after merging this branch until fixing the HTML parsing because the tests will fail and users will be quite annoyed when make test fails! Fix the tests before submitting a new release to CPAN!

If you have any trouble merging feel free to contact me! I'll do the work of rebasing for you so you don't have to fuss with it! Thanks!

@bambams

This comment has been minimized.

Show comment
Hide comment
@bambams

bambams Jan 31, 2015

Contributor

Heads up, I just rebased the branch on your latest master and rewrote the branch with push -f. If you do decide to merge that should make it easier for you.

Contributor

bambams commented Jan 31, 2015

Heads up, I just rebased the branch on your latest master and rewrote the branch with push -f. If you do decide to merge that should make it easier for you.

@bambams

This comment has been minimized.

Show comment
Hide comment
@bambams

bambams Jan 31, 2015

Contributor

Danger Will Robinson: I don't know why, but now the tests introduced by this pull request appear not to be failing properly. Only 1 test is failing, but all 10 of them should fail... I'm not seeing any updates from you to fix the regular expression so I'm at a loss for why the tests aren't failing now. To be fair, I am pretty drunk. ;)

I'll have to look into this to determine why it's not making sense... Perhaps tomorrow once I am rested and sober. In the meantime, this branch might be broken. I don't know if it was caused by the merge+rebase of my other branches or if I made a mistake with the tests... I'll update this thread when I have figured out what went wrong.

Contributor

bambams commented Jan 31, 2015

Danger Will Robinson: I don't know why, but now the tests introduced by this pull request appear not to be failing properly. Only 1 test is failing, but all 10 of them should fail... I'm not seeing any updates from you to fix the regular expression so I'm at a loss for why the tests aren't failing now. To be fair, I am pretty drunk. ;)

I'll have to look into this to determine why it's not making sense... Perhaps tomorrow once I am rested and sober. In the meantime, this branch might be broken. I don't know if it was caused by the merge+rebase of my other branches or if I made a mistake with the tests... I'll update this thread when I have figured out what went wrong.

@bambams

This comment has been minimized.

Show comment
Hide comment
@bambams

bambams Jan 31, 2015

Contributor

Well I figured out what was wrong... I caused a regression in your module! So embarrassing! See here for the fix: #8

Contributor

bambams commented Jan 31, 2015

Well I figured out what was wrong... I caused a regression in your module! So embarrassing! See here for the fix: #8

bambams added some commits Jan 24, 2015

h4x: Add a new test suite for HTML parsing.
These tests are made to fail with the current implementation as a
demonstration of why a proper HTML parser is needed. These tests are a
bit contrived. I have limited experience with test-driven development.
These are just a proof of concept really.

I aim to also submit a patch that uses an HTML parser, but that will be
a separate branch because I'm having trouble figuring out the
appropriate one to use, and in the event that my solution isn't merged
there might still be a chance that the test gets merged.
h4x: Add HTML test dependencies.
It may not be worth the Readonly.pm dependency just for the test when
the tight scope already makes it easy to see that the variables aren't
being changed... I probably am going overboard with Readonly anyway. I
just discovered it so I'm experimenting with its usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment