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

test for "ltx71" robot #113

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@chorny
Contributor

chorny commented Oct 14, 2015

No description provided.

@chorny

This comment has been minimized.

Show comment
Hide comment
@chorny

chorny Oct 14, 2015

Contributor

Even without changes in lib, tests pass for me & Travis-CI, but show that this is not a robot, so either I wrote incorrect test or tests are broken.

Contributor

chorny commented Oct 14, 2015

Even without changes in lib, tests pass for me & Travis-CI, but show that this is not a robot, so either I wrote incorrect test or tests are broken.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Oct 10, 2018

Owner

@chorny thanks for this. My apologies for taking 3 years to take a proper look at this. The problem is that this test which you added is actually not being run (as you suspected). The two JSON files with tests get converted to hashrefs that are then merged into a hash. Tests which exists in both hashes are getting clobbered on the left hand side. So, useragents.json is getting clobbered by more-useragents.json. It looks like this was introduced in cd9ab64, specifically at

my $json = path( "$FindBin::Bin/useragents.json" )->slurp;
my $tests = JSON::PP->new->ascii->decode( $json );
$json = path( "$FindBin::Bin/more-useragents.json" )->slurp;
my $more_tests = JSON::PP->new->ascii->decode( $json );
$tests = { %$tests, %$more_tests };
.

The test case which you've provided is being overridden by:

   "ltx71 - (http://ltx71.com/)" : {
      "browser_beta" : "com/)",
      "browser_major" : "71",
      "browser_minor" : ".0",
      "match" : [],
      "robot" : null
   },

So, a couple of things need to happen here.

  • The tests need to be sorted out (see #143)
  • We need to add bot detection for this bot (see #104)

Once those are sorted we can see about getting this merged.

Owner

oalders commented Oct 10, 2018

@chorny thanks for this. My apologies for taking 3 years to take a proper look at this. The problem is that this test which you added is actually not being run (as you suspected). The two JSON files with tests get converted to hashrefs that are then merged into a hash. Tests which exists in both hashes are getting clobbered on the left hand side. So, useragents.json is getting clobbered by more-useragents.json. It looks like this was introduced in cd9ab64, specifically at

my $json = path( "$FindBin::Bin/useragents.json" )->slurp;
my $tests = JSON::PP->new->ascii->decode( $json );
$json = path( "$FindBin::Bin/more-useragents.json" )->slurp;
my $more_tests = JSON::PP->new->ascii->decode( $json );
$tests = { %$tests, %$more_tests };
.

The test case which you've provided is being overridden by:

   "ltx71 - (http://ltx71.com/)" : {
      "browser_beta" : "com/)",
      "browser_major" : "71",
      "browser_minor" : ".0",
      "match" : [],
      "robot" : null
   },

So, a couple of things need to happen here.

  • The tests need to be sorted out (see #143)
  • We need to add bot detection for this bot (see #104)

Once those are sorted we can see about getting this merged.

This was referenced Oct 10, 2018

@oalders oalders closed this in #147 Oct 11, 2018

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