-
Notifications
You must be signed in to change notification settings - Fork 78
Test: Implemented OOP approach for tested applications in the pytest framework #1304
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
base: main
Are you sure you want to change the base?
Conversation
c225d8a to
1db3b68
Compare
tests/validation/tests/single/st20p/integrity/test_integrity_refactored.py
Outdated
Show resolved
Hide resolved
tests/validation/tests/single/st22p/quality/test_quality_refactored.py
Outdated
Show resolved
Hide resolved
e6991e5 to
5601ccf
Compare
5601ccf to
8c89785
Compare
| "unicast_tx_ip": "192.168.17.101", | ||
| "unicast_rx_ip": "192.168.17.102", | ||
| "multicast_tx_ip": "192.168.17.101", | ||
| "multicast_rx_ip": "192.168.17.102", | ||
| "multicast_destination_ip": "239.168.48.9", |
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.
Please don't hardcode IP addresses. Use IP pools instead.
| rx_process = self.start_process(rx_command, build, test_time, rx_host) | ||
| time.sleep(sleep_interval) | ||
| tx_process = self.start_process(tx_command, build, test_time, tx_host) | ||
| tx_output = self.capture_stdout(tx_process, tx_name) |
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.
there will be no stdout_text untill process is not finished. Ensure that you wait, or kill the process before calling process.stdout_text attrubute
3b7ca6c to
e53377d
Compare
Add parent class for common parameters and execution logic across RxTxApp, FFmpeg, and Gstreamer. Includes RxTxApp child class implementation. Signed-off-by: Wilczynski, Andrzej <[email protected]>
…OOP approach. Signed-off-by: Wilczynski, Andrzej <[email protected]>
…OOP approach. Signed-off-by: Wilczynski, Andrzej <[email protected]>
…n main, cleanup Signed-off-by: Wilczynski, Andrzej <[email protected]>
Signed-off-by: Wilczynski, Andrzej <[email protected]>
Signed-off-by: Wilczynski, Andrzej <[email protected]>
e53377d to
dc0b347
Compare
moleksy
left a comment
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 after changes
DawidWesierski4
left a comment
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.
im generally fine with the overall
i think it would be good to tag all of the shortcomings of the new class-oriented fix
i believe used legacy functions should be just moved into some common folder and or made better and let in the new framework
Especially the add_interfaces irks me the wrong way as its wirdly uses same logic twice?!
I think setting @verified tag would be nice
i generally think this is nitpicking and all of those comments can be ignored as long as the refactored tests are working in NIghtly safely
Great work glhf
| "input_file": "st20p_url", # for input files | ||
| "output_file": "st20p_url", # for output files (RX) |
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.
i guess using input_file as general will not work when working with st30 ?
but im fine with this as is
i would just TODO -> and make the url more general so taht st40 and st30 can be "handled" by this in the future
| "width": "width", | ||
| "height": "height", |
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.
thaaat can be right or can be wrong
If we are taking those from caps its width -> for rx elements its rx-width
i don't think we are enabling any of gstreamer tests with this pr mby just adding a todo to unify this before unlocking the rx side would be enough ?
Or would this work oob ?
Generally todo
I would add the #TOFIX tag above if possible
|
|
||
| # Build command line | ||
| cmd_parts = [ | ||
| "sudo", |
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.
@staszczuk Should sudo be included?
With MFD will it work correctly?
I believe we have some MFD implementation handling the permissions of the process?
| try: | ||
| add_interfaces(config, nic_port_list, test_mode) | ||
| except Exception as e: | ||
| logger.warning( | ||
| f"Legacy add_interfaces failed ({e}); falling back to direct assignment" | ||
| ) | ||
| # Minimal fallback assignment - handle single or dual interface configs | ||
| config["interfaces"][0]["name"] = nic_port_list[0] | ||
| # Set IP addresses based on test mode using ip_pools directly | ||
| if test_mode == "unicast": | ||
| config["interfaces"][0]["ip"] = ip_pools.tx[0] if ip_pools.tx else None | ||
| config["tx_sessions"][0]["dip"][0] = ( | ||
| ip_pools.rx[0] if ip_pools.rx else None | ||
| ) | ||
| config["rx_sessions"][0]["ip"][0] = ( | ||
| ip_pools.tx[0] if ip_pools.tx else None | ||
| ) | ||
| elif test_mode == "multicast": | ||
| config["interfaces"][0]["ip"] = ip_pools.tx[0] if ip_pools.tx else None | ||
| config["tx_sessions"][0]["dip"][0] = ( | ||
| ip_pools.rx_multicast[0] if ip_pools.rx_multicast else None | ||
| ) | ||
| config["rx_sessions"][0]["ip"][0] = ( | ||
| ip_pools.rx_multicast[0] if ip_pools.rx_multicast else None | ||
| ) | ||
| elif test_mode == "kernel": | ||
| config["tx_sessions"][0]["dip"][0] = "127.0.0.1" | ||
| config["rx_sessions"][0]["ip"][0] = "127.0.0.1" | ||
|
|
||
| if len(nic_port_list) > 1: | ||
| config["interfaces"][1]["name"] = nic_port_list[1] | ||
| if test_mode == "unicast": | ||
| config["interfaces"][1]["ip"] = ( | ||
| ip_pools.rx[0] if ip_pools.rx else None | ||
| ) | ||
| elif test_mode == "multicast": | ||
| config["interfaces"][1]["ip"] = ( | ||
| ip_pools.rx[0] if ip_pools.rx else None | ||
| ) | ||
| elif direction in ("tx", "rx"): | ||
| # For single-direction single-interface, remove second interface | ||
| if len(config["interfaces"]) > 1: | ||
| config["interfaces"] = [config["interfaces"][0]] |
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.
logic in error handling is the same line for lline from the legacy,
lets just delete this altogether ? Or try to not use legacy ?
i belive this is kinda confusing in currnet form
Please and thank you
| @pytest.mark.nightly | ||
| @pytest.mark.parametrize( | ||
| "media_file", | ||
| [yuv_files_422p10le["Penguin_1080p"]], | ||
| indirect=["media_file"], | ||
| ids=["Penguin_1080p"], | ||
| ) | ||
| @pytest.mark.parametrize("codec", ["JPEG-XS", "H264_CBR"]) | ||
| def test_codec_refactored( | ||
| hosts, | ||
| build, | ||
| media, | ||
| setup_interfaces: InterfaceSetup, | ||
| test_time, | ||
| codec, | ||
| test_config, | ||
| prepare_ramdisk, | ||
| pcap_capture, | ||
| media_file, | ||
| ): | ||
| media_file_info, media_file_path = media_file | ||
| host = list(hosts.values())[0] | ||
| interfaces_list = setup_interfaces.get_interfaces_list_single( | ||
| test_config.get("interface_type", "VF") | ||
| ) | ||
|
|
||
| app = RxTxApp(app_path="./tests/tools/RxTxApp/build") | ||
| app.create_command( | ||
| session_type="st22p", | ||
| test_mode="multicast", | ||
| nic_port_list=interfaces_list, | ||
| width=media_file_info["width"], | ||
| height=media_file_info["height"], | ||
| framerate=f"p{media_file_info['fps']}", | ||
| codec=codec, | ||
| quality="speed", | ||
| pixel_format=media_file_info["file_format"], | ||
| input_file=media_file_path, | ||
| codec_threads=2, | ||
| test_time=test_time, | ||
| ) | ||
| app.execute_test(build=build, test_time=test_time, host=host, netsniff=pcap_capture) |
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 is just a comment but there is no support for codec meaning
1 . we don't check if the codec is "loaded" in the logs
2. we don't have suppoort for kahawai.json, -- we can't really change it from the framework
would be good to make the #TODO or #FIXME somwhere here
| class RxTxApp(Application): | ||
| """RxTxApp framework implementation (unified model).""" | ||
|
|
||
| def get_framework_name(self) -> str: |
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.
Can we call this get_app_name()? "framework" is confusing in this context.
| # We rebuild the legacy shell for all session types but only populate the active one. | ||
|
|
||
| session_type = self.universal_params.get( | ||
| "session_type", UNIVERSAL_PARAMS["session_type"] |
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.
is using UNIVERSAL_PARAMS["session_type"] as default value makes any sense if self.universal_params is copy of UNIVERSAL_PARAMS? This suggests that self.universal_params can be edited after copy. is this true?
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.
ok. I read some more, and I understand that those params are changed in set_universal_params(). Please rename set_universal_params() and self.universal_params to something like set_params() and self.params. The params are no longer "universal" after set_universal_params()
| @abstractmethod | ||
| def create_command(self, **kwargs): | ||
| """Populate self.command (+ self.config for frameworks that need it). | ||
|
|
||
| Implementations MUST: | ||
| - call self.set_universal_params(**kwargs) | ||
| - set self.command (string) | ||
| - optionally set self.config | ||
| - write config file immediately if applicable | ||
| They MAY return (self.command, self.config) for backward compatibility with existing tests. | ||
| """ | ||
| raise NotImplementedError |
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.
instead of writing all of this in comment I guess it would be better to just implement this method and call from it an abstract version of _create_rxtxapp_command_and_config. in derived classes only this method should be implemented.
| "YUV422RFC4175PG2BE10": "yuv422p10le", # RFC4175 to planar 10-bit LE | ||
| } | ||
|
|
||
| SESSION_TYPE_MAP = { |
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.
is this ever used? _get_session_type_from_config does not use it.
|
|
||
|
|
||
| # Default port configuration by session type | ||
| DEFAULT_PORT_CONFIG = { |
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.
is this ever used?
| } | ||
|
|
||
| # Default ST22p-specific configuration | ||
| DEFAULT_ST22P_CONFIG = { |
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.
is this ever used?
| } | ||
|
|
||
| # Default FFmpeg configuration | ||
| DEFAULT_FFMPEG_CONFIG = { |
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.
is this ever used?
| } | ||
|
|
||
| # Default GStreamer configuration | ||
| DEFAULT_GSTREAMER_CONFIG = {"default_session_type": "mtl_st20p"} |
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.
is this ever used?
| APP_NAME_MAP = {"rxtxapp": "RxTxApp", "ffmpeg": "ffmpeg", "gstreamer": "gst-launch-1.0"} | ||
|
|
||
| # Format conversion mappings | ||
| FFMPEG_FORMAT_MAP = { |
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.
is this ever used?
| from .config.param_mappings import RXTXAPP_CMDLINE_PARAM_MAP | ||
| from .config.universal_params import UNIVERSAL_PARAMS | ||
| from .execute import log_fail | ||
| from .RxTxApp import ( |
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.
Do I understand correctly that the idea of extxapp.py is to replace RxTxApp.py? If we want to remove RxTxApp.py I guess we need to copy those functions
| [ | ||
| yuv_files_422rfc10["Penguin_720p"], | ||
| yuv_files_422rfc10["Penguin_1080p"], | ||
| pytest.param(yuv_files_422p10le["Penguin_720p"], marks=pytest.mark.nightly), |
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.
I'm not sure how this works but isn't @pytest.mark.nightly marking all of those params as nightly and adding marks=pytest.mark.nightly redundant?
| [ | ||
| "p23", | ||
| "p24", | ||
| pytest.param("p25", marks=pytest.mark.nightly), |
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.
I'm not sure how this works but isn't @pytest.mark.nightly marking all of those params as nightly and adding marks=pytest.mark.nightly redundant?
Added parent class for common parameters and execution logic across RxTxApp, FFmpeg, and Gstreamer. Includes RxTxApp child class implementation.