-
Notifications
You must be signed in to change notification settings - Fork 66
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
Use "All" page of requested entity as starting url for session #422
Use "All" page of requested entity as starting url for session #422
Conversation
Reduces individual test run time by 8-13 seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Comment is only a suggestion.
|
||
@cached_property | ||
def virtwho_configure(self): | ||
"""Instance of Virtwho Configure entity.""" | ||
return VirtwhoConfigureEntity(self.browser) | ||
return self._open(VirtwhoConfigureEntity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not a blocker, the PR LGTM.
I only have one comment/suggestion about how it is possible to make it DRY, all that @cached_property
are similiar and too much code repetition.
@lru_cache # lru_cache can replace @cached_property
def get_open_entity(self, name):
# This dict can actually be automatically built from `airgun.entities` members.
entities = {
'virtwho_configure': VirtwhoConfigureEntity,
'usergroup': UserGroupEntity,
...
}
return self._open(entities[name])
def __getattr__(self, name):
"""Called when an attribute lookup has not found"""
try:
return self.get_open_entity(name)
except KeyError:
return super().__getattr__(name)
That is just an idea to be tested/evaluated and maybe implemented in another PR because with this it is possible to replace all the repetition, 248 lines becomes 72 lines. (or less)
Any time a session.foo
is called, the foo
is going to be looked up in the entity list before it raises AttributeError
if not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rochacbruno looks like to be a good idea not sure here about consequences, unrelated to this PR. can you please create an issue in Airgun with mentioning details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, both idioms below are supported: with session: # do something with session(url='url'): # open $HOSTNAME/url and do something Initially, we wanted tests to only submit new URL path, but since it's easy to extend that to other arguments, why not do that? Also, actually don't open browser if session.entity is never invoked in test.
With bd1653b, both of following idioms are supported: with session:
# do something - starting URL will be read from requested entity with session(url="url"):
# do something - starting URL will be $HOSTNAME/url, i.e. provided by test and not entity We initially discussed I am very much interested on what you think about this API. |
@mirekdlugosz overall looks nice, we are adding many ways access session makes more sense to me. I have one doubt here If we use plain In other cases with |
With plain
I have checked on
Of course these are only single runs for each. So yes, it will reduce execution time a little further if we change robottelo to set initial URL to that of "new entity" page. But that's for another PR, as it must be done in robottelo and not airgun. |
Absolutely, thanks for examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
with a question comment for @mirekdlugosz
140334a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
This is Airgun-only re-implementation of idea presented by @omkarkhatavkar in SatelliteQE/robottelo#7442
The main goal here is to use entity "All items" URL as starting point for session. This way list of all entities will be visible right after logging in (instead of displaying dashboard, which we hardly ever need).
This is implemented by deferring execution of browser initialization code until entity is requested. It has two effects:
Session
context manager, but never actually interact with it, browser will not be initialized. This will save time during test development and allows us to put more code inside session context (as there is no performance penalty for that).Test case:
Before change:
After change:
So, with that PR, we can expect individual test execution time to be reduced by 8 to 13 seconds.
I am also running entire UI Robottelo suite, but this takes forever.