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

Support for robotframework 4.x #21

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Conversation

matejc
Copy link
Contributor

@matejc matejc commented Aug 2, 2021

Add support for robotframework 4.x alongside with previous versions.

@matejc matejc requested a review from Tattoo August 2, 2021 10:00
@matejc matejc marked this pull request as draft August 2, 2021 11:55
Copy link
Contributor

@Tattoo Tattoo left a comment

Choose a reason for hiding this comment

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

Probably have to talk face-to-face about one of the comments in the interface

Comment on lines 6 to 7
__all__ = ['BaseHandler', 'listener', 'OxygenLibrary', 'RobotInterface',
'get_keywords_from']
Copy link
Contributor

Choose a reason for hiding this comment

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

__all__ should be used to declare the public interface of the module. get_keywords_from is not intended to be called from outside of Oxygen, so it should not be defined in __all__. Nor should RobotInterface for that matter; the old code was wrong as well.

Comment on lines 13 to 15
SETUP_TYPE = RobotResultKeyword.SETUP_TYPE
KEYWORD_TYPE = RobotResultKeyword.KEYWORD_TYPE
TEARDOWN_TYPE = RobotResultKeyword.TEARDOWN_TYPE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing the old RF3 interface in any way? To my mind, it should not need to change at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can see, these are needed only for tests. I do not quite see why we would change the interface to appease tests; rather, the tests should be re-thought to verify correct things.

Comment on lines 373 to 375
KEYWORD_TYPE = RobotResultKeyword.KEYWORD_TYPE
SETUP_TYPE = RobotResultKeyword.SETUP_TYPE
TEARDOWN_TYPE = RobotResultKeyword.TEARDOWN_TYPE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we exposing this new functionality? What outside of interface needs this information?

@matejc matejc force-pushed the feat/robotframework-4-support/pr branch from 1d6f479 to 0817e67 Compare August 26, 2021 14:07
@matejc matejc marked this pull request as ready for review August 26, 2021 14:07
@matejc matejc requested a review from Tattoo August 26, 2021 14:07
@matejc matejc force-pushed the feat/robotframework-4-support/pr branch from 0817e67 to c4a9a3f Compare August 26, 2021 15:01
Copy link
Contributor

@Tattoo Tattoo left a comment

Choose a reason for hiding this comment

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

Apparently Github does not allow me to make comments/suggestions on lines that have not been changed, so let's do this the stupid way then ☹️

Please also fix the problems in RobotInterfaceBasicTests we discussed yesterday:

~line 166:

for subsuite in converted[0].suites:
            self.assertIsInstance(converted_suite, RobotSuite)
# ^ this should be:
for subsuite in converted[0].suites:
            self.assertIsInstance(subsuite, RobotSuite)

~line 171

for kw in get_keywords_from(converted_suite.tests[1]):
            self.assertIsInstance(kw, RobotKeyword)
# ^ this should be:
for kw in get_keywords_from(converted[0].tests[1]):
            self.assertIsInstance(kw, RobotKeyword)

~line 174:

for message in get_keywords_from(converted_suite.tests[1])[0].messages:
            self.assertIsInstance(message, RobotMessage)
# ^ this should be:
for message in get_keywords_from(converted[0].tests[1])[0].messages:
            self.assertIsInstance(message, RobotMessage)


__all__ = ['BaseHandler', 'listener', 'OxygenLibrary', 'RobotInterface']
__all__ = ["BaseHandler", "listener", "OxygenLibrary"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong quotes, should be single quotes as anywhere else

@matejc matejc force-pushed the feat/robotframework-4-support/pr branch from c4a9a3f to 8b426ee Compare September 7, 2021 11:04
@matejc matejc requested a review from Tattoo September 7, 2021 11:04
Copy link
Contributor

@Tattoo Tattoo left a comment

Choose a reason for hiding this comment

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

LGTM 💖

@matejc matejc merged commit c21beb0 into master Sep 8, 2021
@matejc matejc deleted the feat/robotframework-4-support/pr branch September 8, 2021 08:03
@Tattoo Tattoo mentioned this pull request Nov 6, 2021
@Tattoo
Copy link
Contributor

Tattoo commented Nov 6, 2021

Fixes #18

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.

2 participants