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

Add squishserver module #18

Merged
merged 17 commits into from
Jun 15, 2023
Merged

Add squishserver module #18

merged 17 commits into from
Jun 15, 2023

Conversation

ghost
Copy link

@ghost ghost commented Apr 28, 2023

Added squishserver module to work with squishserver, which contains a dedicated class SquishServer. Added 3 methods to register AUTs:

  • addAUT
  • addAppPath
  • addAttachableAut
    Also added a dedicated exception type "SquishserverError"

Added squishserver module to work with squishserver, that contains a dedicated class SquishServer.
Added 3 methods to register AUTs:
- addAUT
- addAppPath
- addAttachableAut
Also added dedicated exception type "SquishserverError"
@ghost ghost requested a review from pawtom April 28, 2023 14:32
@Adakar Adakar marked this pull request as draft May 2, 2023 10:16
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Show resolved Hide resolved
@pawtom
Copy link
Contributor

pawtom commented May 9, 2023

For the completeness of the solution, it might be worth to add "remove" functions for the "add" functions:
*removeAUT
*removeAppPath
*removeAttachableAut

@ghost ghost self-assigned this May 10, 2023
@pawtom
Copy link
Contributor

pawtom commented May 10, 2023

With the recent changes, one can see the need to create a helper function, like:
_config_squishserver(config_option:str, params=[]) and extract common code from six created API functions into that one.

@pawtom
Copy link
Contributor

pawtom commented May 10, 2023

Thanks for the recent changes, looks like a big improvement :)

@ghost ghost requested review from Adakar and pawtom May 10, 2023 14:47
Copy link
Contributor

@Adakar Adakar left a comment

Choose a reason for hiding this comment

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

@pawtom could you please check my comments? :)

squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Show resolved Hide resolved
squape/squishserver.py Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
@ghost ghost requested a review from Adakar May 11, 2023 10:11
Copy link
Contributor

@Adakar Adakar left a comment

Choose a reason for hiding this comment

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

In general it would be good to see squishserver details in logs. Lets imagine a situation where you have 2 squishservers. If you register different AUTs in each of them, the logs do not say which AUT was added to which squishserver

squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Show resolved Hide resolved
@ghost ghost requested a review from Adakar May 16, 2023 09:56
Copy link
Contributor

@Adakar Adakar left a comment

Choose a reason for hiding this comment

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

Please read the comment I made last time

squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Show resolved Hide resolved
@Adakar Adakar added the enhancement New feature or request label May 21, 2023
@ghost ghost requested a review from Adakar May 25, 2023 08:53
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
@pawtom
Copy link
Contributor

pawtom commented May 26, 2023

@Adakar @EduardKaverinskyi
I'm wondering about the default port for squishserver.
def __init__(self, location=None, host="127.0.0.1", port=4322):
When squishserver is started automatically, this information is stored in SQUISHRUNNER_PORT environment variable.

So instead of doing something like that in the test script:
ss_port = os.environ.get("SQUISHRUNNER_PORT",4322) SquishServer(port=ss_port).addAttachableAut("abab", 5858, "localhost")

I would suggest that for the init method default values are:
for host
1. SQUISHRUNNER_HOST
2. localhost (if SQUISHRUNNER_HOST is not defined)

for port:
1. SQUISHRUNNER_PORT
2. 4322

What do you think?

@ghost
Copy link
Author

ghost commented May 31, 2023

I think that this idea is usable. I implemented it in the latest commit. Please check if it is as is suggested.

@ghost ghost requested a review from pawtom May 31, 2023 10:55
Copy link
Contributor

@Adakar Adakar left a comment

Choose a reason for hiding this comment

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

@EduardKaverinskyi I'll not accept this PR without implementing more verbose log messages. It was mentioned twice already in this comment

squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Show resolved Hide resolved
squape/squishserver.py Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
squape/squishserver.py Outdated Show resolved Hide resolved
@Adakar Adakar marked this pull request as ready for review June 15, 2023 13:02
@Adakar Adakar marked this pull request as draft June 15, 2023 13:12
Changes:
- Add docstrings to the class and methods
- Extend the SquishserverError message to include additional data about failure
- Format the code
- Reorder the imports
@ghost ghost force-pushed the squishserver_module_branch branch from e567093 to bd6ef03 Compare June 15, 2023 13:35
@Adakar Adakar marked this pull request as ready for review June 15, 2023 13:48
@Adakar Adakar merged commit 2ba2b10 into main Jun 15, 2023
@Adakar Adakar deleted the squishserver_module_branch branch June 15, 2023 13:53
Adakar pushed a commit that referenced this pull request Jul 4, 2023
* Add squishserver module

Allows attaching to and starting AUT

Allows configuring squishserver:
- addAUT
- removeAUT
- addAppPath
- removeAppPath
- addAttachableAut
- removeAttachableAut

---------
Co-authored-by: TomaszPe <tomasz.pawlowski@gmail.com>
Adakar pushed a commit that referenced this pull request Aug 22, 2023
* Add squishserver module

Allows attaching to and starting AUT

Allows configuring squishserver:
- addAUT
- removeAUT
- addAppPath
- removeAppPath
- addAttachableAut
- removeAttachableAut

---------
Co-authored-by: TomaszPe <tomasz.pawlowski@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants