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

Port Crawler Improvements #258

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Port Crawler Improvements #258

wants to merge 3 commits into from

Conversation

Shashi456
Copy link
Contributor

Fixes #236

@codecov-io
Copy link

codecov-io commented Aug 15, 2018

Codecov Report

Merging #258 into master will decrease coverage by 0.19%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #258     +/-   ##
=========================================
- Coverage   15.83%   15.64%   -0.2%     
=========================================
  Files          13       13             
  Lines        1894     1918     +24     
  Branches      328      331      +3     
=========================================
  Hits          300      300             
- Misses       1592     1616     +24     
  Partials        2        2
Impacted Files Coverage Δ
collect.py 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7773a9...c13f5a8. Read the comment docs.

collect.py Outdated

from autowebcompat import utils

already_clicked_elems = set()
Copy link
Owner

Choose a reason for hiding this comment

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

We have several crawler threads, so this can't be a global variable (otherwise the different threads will overwrite each other's variable).

Copy link
Owner

@marco-c marco-c left a comment

Choose a reason for hiding this comment

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

Just the concurrency problem to fix as far as I can see.

@sagarvijaygupta do you want to take a look too?

collect.py Outdated
break
else:
children_to_ignore.extend(elems)
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@marco-c This else is not aligned correctly with its corresponding if

@Shashi456
Copy link
Contributor Author

@sagarvijaygupta @marco-c so i made already_clicked_elems a local variable and took care of indentation, there are redundancies in the exception blocks because we check if the xpath exists or not, so there's a try-catch block for each conditional.

@marco-c
Copy link
Owner

marco-c commented Aug 27, 2018

there are redundancies in the exception blocks because we check if the xpath exists or not, so there's a try-catch block for each conditional.

What if you put the try before the if-else for xpath?

@marco-c
Copy link
Owner

marco-c commented Aug 27, 2018

so i made already_clicked_elems a local variable

It can't be local to do_something, otherwise it gets cleared every time we click on something and thus it becomes ineffective.

@Shashi456
Copy link
Contributor Author

@marco-c where should i declare it instead? because one way i can think of is declaring it in the function which is calling it and pass it as a parameter

@sagarvijaygupta
Copy link
Collaborator

@Shashi456 I think we should declare it in run_test_both and pass it to jump_back also.

@marco-c
Copy link
Owner

marco-c commented Sep 6, 2018

@Shashi456 I think we should declare it in run_test_both and pass it to jump_back also.

Yes, we should declare it in run_test_both.
Do we need it for jump_back? In jump_back we should already know exactly what we're going to click, right?

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.

4 participants