Draft PR to get feedback on first tests #292

JuliaS92 · 2024-08-29T14:02:48Z

I have used the streamlit testing library in combination with mocking of file inputs (thank you @mschwoer for help with that).

I now have 4 tests so far:

page loads
page loads with mocking
sample dataset loads
maxquant testdata (proteinGroups and metadata) loads

In each case I have only asserted the session state and not what the interface actually displays.

This way, we can test if the app runs through and can also make sure that what's persistent between pages is correct, while not affecting testing by aestetic changes to the interface.

I would further be happy to receive suggestions on how to best structure the tests to not duplicate too much code, if I add a test for each software option. (A link to a good example for shared/parameterized setup would be appreciated.)

I will also add tests to make sure the procedurally generated widgets are populated correctly, unless you think this is unnecessary work.

I would not go into testing different input combinations too much, as this is covered by the loader unittests.

…tion.

mschwoer

🚀

alphastats/gui/AlphaPeptStats.py

alphastats/gui/test/test_02_Import Data.py

mschwoer · 2024-08-30T06:53:17Z

alphastats/gui/utils/analysis_helper.py

@@ -134,7 +134,7 @@ def st_general(method_dict):

 @st.cache_data
 def gui_volcano_plot_differential_expression_analysis(
-    chosen_parameter_dict, user_session_id=st.session_state.user_session_id
+    chosen_parameter_dict, user_session_id
 ):
    """
    initalize volcano plot object with differential expression analysis results


adressing your first question:

"how to best structure if I add a test for each software option. (A link to a good example for shared/parameterized setup would be appreciated.)"

There is pytest.parametrize, but I'm afraid this will become quite nasty once e.g. the input fields differ depending on the options..
Maybe we should aim at unifying the workflows for the different softwares as much as possible to faciliate testing (and have less complex code).. e.g. by always displaying all fields, and just disabling those irrelevant for the current selection?

So the number of fields displayed is always the same and it is just a matter of populating the options after the file is uploaded. I will change this the behaviour to show the whole interface (both file_loaders and all three dropdowns) at once and then just update the options when the files are loaded.

if this looked like this (pseudocode)

value1, value2, value3 = get_values_for_populating_options(..., selected_software="something") st.field1.value = value1 st.field2.value = value2 ...

then 1 UI test with a mocked get_values_for_populating_options that checks that the fields are correctly populated would suffice ..
get_values_for_populating_options could then be tested outside of the streamlit testing framework

mschwoer · 2024-08-30T06:56:13Z

alphastats/gui/utils/analysis_helper.py

@@ -134,7 +134,7 @@ def st_general(method_dict):

 @st.cache_data
 def gui_volcano_plot_differential_expression_analysis(
-    chosen_parameter_dict, user_session_id=st.session_state.user_session_id
+    chosen_parameter_dict, user_session_id
 ):
    """
    initalize volcano plot object with differential expression analysis results


addressing your second question:

I will also add tests to make sure the procedurally generated widgets are populated correctly, unless you think this is unnecessary work.

From a vertical slicing perspective: instead of testing page 02 super-thoroughly, alternatively you could first add test like these (page loads, basic user flow works) for the other pages ..
I think these kind of tests already add a lot of value. Mb add a TODO here to make transparent that the tests could be extented in terms of scope.

mschwoer · 2024-08-30T07:00:16Z

alphastats/gui/test/test_02_Import Data.py

+    assert at.session_state.dataset.sample == 'sample'
+    assert str(type(at.session_state.distribution_plot)) == "<class 'plotly.graph_objs._figure.Figure'>"
+    assert str(type(at.session_state.loader)) == "<class 'alphastats.loader.MaxQuantLoader.MaxQuantLoader'>"
+    assert "plotting_options" in at.session_state


just FYI: if possible, aim for testing the specific values (assert at.session_state["plotting_options"] == {...}, rather than just checking if the key exists.. makes the test stronger

This was my way of vertical slicing ;) The values of these operations are actually tested elsewhere, although I don't have a full overview yet. For now this is just to make sure the function returned the correct object and not None or an error.

…simulated user action

mschwoer

LGTM! some comments around the consistent use of pathlib.Path

mschwoer · 2024-09-03T08:21:02Z

alphastats/gui/pages/02_Import Data.py

@@ -207,6 +210,8 @@ def upload_softwarefile(software):
            st.session_state["loader"] = loader


+# TODO: Move this to new data_helper.py in utils
+# TODO: Make this more powerful by making suggestions based on the sample names


The "more powerful" TODO I would move to a ticket

mschwoer · 2024-09-03T08:23:00Z

alphastats/gui/test/test_02_import_data.py

+
+APP_FOLDER = Path(__file__).parent / Path("../")
+APP_FILE = f"{APP_FOLDER}/pages/02_Import Data.py"
+TEST_INPUT_FILES = f"{APP_FOLDER}/../../testfiles"


`TEST_INPUT_FILES_PATH = APP_FOLDER / "../../testfiles"

mschwoer · 2024-09-03T08:23:11Z

alphastats/gui/test/test_02_import_data.py

+        print(f"{k}:    {str(type(v))}   {str(v)[:20] if type(v) not in [int, list, str] else v}")
+
+APP_FOLDER = Path(__file__).parent / Path("../")
+APP_FILE = f"{APP_FOLDER}/pages/02_Import Data.py"


could also use Path concatenation with / here

Yes this could be brittle. Mb use os.pardir etc.

mschwoer · 2024-09-03T08:23:28Z

alphastats/gui/test/test_02_import_data.py

+    Additionally add filename as attribute."""
+    with open(f"{TEST_INPUT_FILES}{path_from_testfiles}", "rb") as f:
+        buf = BytesIO(f.read())
+        buf.name = path_from_testfiles.split('/')[-1]


could use pathlib here: .stem or .name or smth

(and in the with statement)

mschwoer · 2024-09-03T08:24:11Z

alphastats/gui/test/test_02_import_data.py

+    assert "statistic_options" in at.session_state
+
+
+def _data_buf(path_from_testfiles: str):


(nit) return value type hint

mschwoer · 2024-09-03T08:25:01Z

alphastats/gui/test/test_02_import_data.py

+import pandas as pd
+from io import BytesIO
+
+def print_session_state(apptest: AppTest):


is this used somewhere? also, redefined in test_03...

straussmaximilian

Thanks for taggin me.
App-Testing in streamlit is very nice and I was not aware of this :)

alphastats/gui/AlphaPeptStats.py

straussmaximilian · 2024-09-03T14:53:18Z

alphastats/gui/pages/03_Data Overview.py

@@ -2,14 +2,14 @@
 import pandas as pd
 import plotly.express as px

-
+#TODO: Cache the whole interface, not each individual plot.


One probably needs to double check what exactly you want to do. Streamlits execution logic reruns from top to down and keeping the settings, so in some sense the options/interface is cached by default.

straussmaximilian · 2024-09-03T14:54:13Z

alphastats/gui/test/test_02_import_data.py

+        print(f"{k}:    {str(type(v))}   {str(v)[:20] if type(v) not in [int, list, str] else v}")
+
+APP_FOLDER = Path(__file__).parent / Path("../")
+APP_FILE = f"{APP_FOLDER}/pages/02_Import Data.py"


Yes this could be brittle. Mb use os.pardir etc.

JuliaS92 added 7 commits August 28, 2024 13:46

Make pages mock test compatible

ee5d93e

enable import without session state

b9baab4

Add simple loading test for data import page

2db4976

remove session_info as this is not mock compatible

d18c91a

add assertions for load test and test for sample data loading

e40cbbb

Add test for mocked input page

f588fa0

Test for loading maxquant testing data without change of column selec…

215c8ea

…tion.

JuliaS92 requested review from mschwoer and boopthesnoot August 29, 2024 14:05

mschwoer reviewed Aug 30, 2024

View reviewed changes

JuliaS92 added 13 commits August 30, 2024 10:26

Rename test file

b9f4ce1

rename mock to match object name

dde5188

Made tests more speaking by name changes, docstrings and comments on …

3188e72

…simulated user action

Note to self: Save before committing

f5c827b

Extract bytebuffer functions

19e390a

Make those private and add docstrings

01ee105

Address create dataset button by key

4617286

Add key to load sample data button

e71b434

Rename TEST_FILES

b29c67a

Add TODOs

cf9f6a6

Removed session dependent default arguments from page 3

bb870ad

Added assert not exception statements to page 2 tests

b9be51d

Page 3 run test

2be07c5

JuliaS92 requested a review from straussmaximilian August 30, 2024 13:06

TODO Page reduce caching

760a52c

JuliaS92 marked this pull request as ready for review September 2, 2024 11:23

JuliaS92 requested a review from mschwoer September 2, 2024 11:24

mschwoer approved these changes Sep 3, 2024

View reviewed changes

straussmaximilian approved these changes Sep 3, 2024

View reviewed changes

JuliaS92 closed this Sep 6, 2024

JuliaS92 deleted the pagetests branch September 6, 2024 12:23

JuliaS92 mentioned this pull request Sep 6, 2024

Basic tests for page 2 and 3 #305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft PR to get feedback on first tests #292

Draft PR to get feedback on first tests #292

JuliaS92 commented Aug 29, 2024

mschwoer left a comment

mschwoer Aug 30, 2024

JuliaS92 Aug 30, 2024

mschwoer Aug 30, 2024

mschwoer Aug 30, 2024

mschwoer Aug 30, 2024

JuliaS92 Aug 30, 2024

mschwoer left a comment

mschwoer Sep 3, 2024

mschwoer Sep 3, 2024

mschwoer Sep 3, 2024

straussmaximilian Sep 3, 2024

mschwoer Sep 3, 2024

mschwoer Sep 3, 2024

mschwoer Sep 3, 2024

straussmaximilian left a comment

straussmaximilian Sep 3, 2024

straussmaximilian Sep 3, 2024

		assert "statistic_options" in at.session_state


		def _data_buf(path_from_testfiles: str):

Draft PR to get feedback on first tests #292

Draft PR to get feedback on first tests #292

Conversation

JuliaS92 commented Aug 29, 2024

mschwoer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mschwoer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straussmaximilian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment