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.

Sign up for GitHub

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

Jump to bottom

Making process_watchdog part of manager_param and removed a process_watchdog check to reflect current state of watchdogs in OpenWPM #787

Merged
merged 16 commits into from
Nov 13, 2020
Merged

Making process_watchdog part of manager_param and removed a process_watchdog check to reflect current state of watchdogs in OpenWPM #787

merged 16 commits into from
Nov 13, 2020

Conversation

ankushduacodes
Copy link
Contributor

fixes #749

vringar
vringar requested changes Nov 11, 2020
View reviewed changes
Copy link
Contributor

@vringar vringar left a comment

Choose a reason for hiding this comment

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

This looks good, except for one small change

docs/Platform-Architecture.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 11, 2020 •
edited
Loading

Codecov Report

Merging #787 (8fd91a0) into master (65337e0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #787      +/-   ##
==========================================
- Coverage   40.06%   40.05%   -0.01%     
==========================================
  Files          29       29              
  Lines        3120     3118       -2     
==========================================
- Hits         1250     1249       -1     
+ Misses       1870     1869       -1
Impacted Files Coverage Δ
automation/TaskManager.py 73.50% <100.00%> (+0.14%) ⬆️

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 65337e0...ff469ca. Read the comment docs.

@ankushduacodes @vringar
Removing an outdated TODO
58e804d 
Co-authored-by: Stefan Zabka <zabkaste@informatik.hu-berlin.de>
@ankushduacodes
Copy link
Contributor Author

ankushduacodes commented Nov 11, 2020 •
edited
Loading

@vringar I think I forgot to run pre-commit before committing and pushing this branch and because of which it failed to run the tests. Is there a way to run the tests again or do I have to make a change to make tests run again?

@vringar
Copy link
Contributor

vringar commented Nov 11, 2020

@ankushduacodes I'd suggest running pre-commit run --all and then commit the changes made and push those. The new commit should make the tests rerun.

@ankushduacodes
Copy link
Contributor Author

@ankushduacodes I'd suggest running pre-commit run --all and then commit the changes made and push those. The new commit should make the tests rerun.

@vringar There were no changes made after I ran pre-commit run --all on my local machine.

@ankushduacodes
Copy link
Contributor Author

Hmmm, I am not sure what is happening, even if I remove the TODO comment, it seem to somehow come back... This is third time I have removed it.... 🤔🤔

@vringar
Copy link
Contributor

vringar commented Nov 11, 2020

I'm looking at the commit history and can't explain it to myself either. If figuring this out is too much of a headache I can also merge it into a temporary branch, make the changes needed and then merge it myself.

@ankushduacodes
Copy link
Contributor Author

I'm looking at the commit history and can't explain it to myself either. If figuring this out is too much of a headache I can also merge it into a temporary branch, make the changes needed and then merge it myself.

For sure, If you need me to do something I will be happy to do it. 😄

@ankushduacodes
Copy link
Contributor Author

I think latest commit did detele, but I should mention that I am not sure what was causing this issue as my both this branch and my forked master branch are even with mozilla/OpenWPM master branch

vringar
vringar requested changes Nov 11, 2020
View reviewed changes
automation/TaskManager.py Show resolved Hide resolved
vringar
vringar reviewed Nov 11, 2020
View reviewed changes
__init__.py Outdated Show resolved Hide resolved
ankushduacodes and others added 2 commits November 11, 2020 20:25
@ankushduacodes @vringar
Update __init__.py
f2e04c0 
Co-authored-by: Stefan Zabka <zabkaste@informatik.hu-berlin.de>
@ankushduacodes @vringar
Update automation/TaskManager.py
19de9af 
Co-authored-by: Stefan Zabka <zabkaste@informatik.hu-berlin.de>
@ankushduacodes
Copy link
Contributor Author

I think I got it, I was removing the wrong comment by accident and When you were suggesting a change I was coming back with that change. I totally mixed two comments without looking into it too much, I am sorry for all the confusion 😅

@vringar
Copy link
Contributor

vringar commented Nov 11, 2020

No worries. I'm just going to wait for the tests to pass and then merge this

@vringar vringar self-requested a review November 13, 2020 14:51
vringar
vringar reviewed Nov 13, 2020
View reviewed changes
automation/TaskManager.py Outdated Show resolved Hide resolved
@vringar vringar merged commit 2a971d0 into openwpm:master Nov 13, 2020
Zaxeli pushed a commit to Zaxeli/OpenWPM that referenced this pull request Aug 10, 2021
@ankushduacodes
Moved process_watchdog and memory_watchdog into manager_params(openwp…
fe64e20 
…m#787)

fixes openwpm#749
Co-authored-by: vringar <szabka@mozilla.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vringar vringar vringar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reactivate process_watchdog now that it's fixed
2 participants
@ankushduacodes @vringar