posix4e / puppet

MIT License
8 stars 8 forks source link

Llm adblock #46

Closed posix4e closed 1 year ago

posix4e commented 1 year ago

I haven't carefully reviewed yet, but it looks like we an amazing start.

Ported from https://github.com/posix4e/puppet/pull/45.

TODO

posix4e commented 1 year ago

Ported from https://github.com/posix4e/puppet/pull/45

github-actions[bot] commented 1 year ago

LOGAF Level 3 - /home/runner/work/puppet/puppet/backend/models.py

  1. The User model has a name field with a server default of "Default name". This could lead to confusion if a user is created without a name. Consider making the name field required.

  2. The __repr__ methods are not returning valid strings. This could lead to errors when trying to print the objects. Make sure to return valid strings.

Example:

def __repr__(self):
    return f"User(id={self.id}, name={self.name}, uid={self.uid})"

LOGAF Level 2 - /home/runner/work/puppet/puppet/tests/test_sauce_labs.py

1. Code Duplication: There is a lot of code duplication in the save_server_settings and enable_accessibility_settings functions. You can create a helper function to find and click an element by ID or XPATH. This will make your code DRY (Don't Repeat Yourself) and easier to maintain.

Example:

def find_and_click_element(driver, by_selector, selector_value):
    element = find_and_click(driver, by_selector, selector_value)
    return element

2. Hardcoded Values: The server URL is hardcoded in the save_server_settings function. It's better to move this to a configuration file or environment variable.

3. Unnecessary Comments: There are several comments in the code that do not add value or explain complex code. Removing these comments can improve code readability.

4. Magic Strings: There are several strings that are used multiple times throughout the code (e.g., AppiumBy.ID, AppiumBy.XPATH). Consider defining these as constants at the top of your file.

5. Unused Code: There are several lines of code that are commented out. If this code is not needed, it should be removed to improve readability.


LOGAF Level 2 - /home/runner/work/puppet/puppet/backend/backend.py

  1. Avoid using print statements for logging. Use a logging library instead. This will give you more control over your logs and you can easily switch between different levels of logging.

  2. The openai_key is being stored in the User model. This could be a potential security risk if the database is compromised. Consider using a more secure method to store sensitive data.

  3. The add_command function commits to the database for each new command. This could be inefficient if there are many commands. Consider adding all the commands first and then commit once.

Example:

commands = [Command(uid=item.uid, command=cmd) for cmd in command_list]
db.add_all(commands)
db.commit()
  1. The send_event function opens a file for each event. This could be inefficient if there are many events. Consider opening the file once and writing all the events.

Example:

with open(f"{item.uid}_events.txt", "a") as f:
    for event in events:
        f.write(f"{datetime.now()} - {event}\n")

๐Ÿ”’๐Ÿ”๐Ÿงน


Powered by Code Review GPT

urbit-pilled commented 1 year ago

looks like saucelabs is still using the old apk file in the E2E tests and it looks like the E2E tests gets triggered twice (push and pull_request)

vkolgi commented 1 year ago

@urbit-pilled that's right. I am working on a PR which will help test the android app at PR level, before merging to master. This should be done by EOD.

Are these tests running in your local with android app ? There is a flag in tests to run it against your local appium server.

Since I do not want you to be blocked on your work, we can temporarily upload this PR specific APK to saucelabs and make tests run against it. That however means uploading it manually every time there is a change in android code.

urbit-pilled commented 1 year ago

@vkolgi Yea, I saw the flag to run appium locally, and I've been using that to write the selenium tests. I'm fine with the temporary solution of running tests against the current apk build, I don't think the apk is going to be changed much after the latest commit (for this PR) and I'll request an apk re-upload if I make any breaking changes.

vkolgi commented 1 year ago

@urbit-pilled It's now handled in another PR. Will wait for @posix4e to have a look at the PR. So, once that's approved, you should be able to rebase and have the tests running against PR specific apk.

vkolgi commented 1 year ago

@urbit-pilled the PR has been merged now. Can you bring in those changes to your branch?

urbit-pilled commented 1 year ago

@posix4e PR is ready to be merged

vkolgi commented 1 year ago

@urbit-pilled I am testing this out locally. Just give me a day to finalize.

vkolgi commented 1 year ago

Also the app while running test is crashing. Attaching the video here. @posix4e As soon as the test pressed the button Turn Off VPN, app crashed. Attached crash log below: At timestamp 0.31sec the app crashes and android home screen appears.

java.lang.InterruptedException
    at java.lang.Thread.sleep(Native Method)
    at java.lang.Thread.sleep(Thread.java:442)
    at java.lang.Thread.sleep(Thread.java:358)
    at com.ttt246.puppet.AdblockVPNThread.run(AdblockVPNThread.kt:103)
    at java.lang.Thread.run(Thread.java:923)

https://github.com/posix4e/puppet/assets/2462783/ecc891ce-4eb7-46a1-aaad-3fa45b172d8b

urbit-pilled commented 1 year ago

I think that's just the end of the unit test, the test presses the start vpn button, then the stop vpn button and then it automatically closes the program after the unit test is done.

vkolgi commented 1 year ago

@urbit-pilled at the end of the test, the app is crashing. So it looks like test is done but the app crashes as soon as disable VPN is pressed. Attaching the screenshot from the saucelabs here.

Screenshot 2023-09-02 at 10 05 59 PM
urbit-pilled commented 1 year ago

You are right @vkolgi , I should have handled the InterruptedException better. Since I don't have saucelabs can you check to see if the latest commit fixes this?

} catch (e: InterruptedException) {
            Log.i(TAG, "Vpn Thread interrupted")
            //throw e <--- deleted this and instead reinterrupted the thread
            Thread.currentThread().interrupt();

https://github.com/posix4e/puppet/commit/c4de696586998cd0cd4a8282bec5168ad3eff519#diff-b0aa8eda0216e995473db3013d9cbd3e36c5c831e737ae601afcb42c5f4c3619R108-R109