Replace: occurrences of tests.utils.wait_until_signal with qtbot.wait_signal

[rohit2p]

Purpose of PR?: This issue concerns a potential race condition when using the wait_until_signal function for Qt testing. The function has a limitation where it only starts listening for a specific signal after the code that should emit that signal has been called. so, we'll Replace occurrences of tests.utils.wait_until_signal with qtbot.wait_signal. Because wait_signal is a context manager from pytest-qt that does not have this issue.

Fixes #2156

[rohit2p]

@matrss so, I've created this PR to address and have attempted to modify the query_server function. However, I'm encountering some roadblocks related to the purpose and usage of certain elements:

1. Uncertainty about self.wms_control.cpdlg.canceled: : I'm unsure what this part of the code represents and its purpose within the query_server function.
2. Confusion about with qtbot.wait_signal(self.wms_control.cpdlg.canceled): block: like what will actually go inside this block.

While I've made changes from what i understood like the primary goal of the test in this change i did is to verify that the application handles user cancellation appropriately. i don't think i made the correct changes. Ik that their are some other changes that is required but i held off thinking i should make things clear before going further. could you guide me a bit.

[matrss]

1. Uncertainty about self.wms_control.cpdlg.canceled: : I'm unsure what this part of the code represents and its purpose within the query_server function.

self.wms_control.cpdlg.canceled is a signal of the QProgressDialog that is defined here:

MSS/mslib/msui/wms_control.py

Lines 519 to 521 in b8a200d

	self.cpdlg = QtWidgets.QProgressDialog(
	"retrieving wms capabilities...", "Cancel", 0, 10, parent=self.multilayers)
	self.cpdlg.canceled.connect(self.stop_capabilities_retrieval)

Waiting for this signal to be emitted is used as an indicator that the retrieval that is associated with this progress dialog has finished. I.e. it is used in the query_server method to wait until all actions have been fully processed, essentially.

2. Confusion about with qtbot.wait_signal(self.wms_control.cpdlg.canceled): block: like what will actually go inside this block.

In the case of the method that you have already modified it would be this line that needs to be wrapped with the context manager:

MSS/tests/_test_msui/test_sideview.py

Line 152 in 2adcb27

QtTest.QTest.mouseClick(self.wms_control.multilayers.btGetCapabilities, QtCore.Qt.LeftButton)

(Maybe also the line before that one, but I don't think so, would need to see if tests pass or fail)

While I've made changes from what i understood like the primary goal of the test in this change i did is to verify that the application handles user cancellation appropriately. i don't think i made the correct changes.

No, the query_server method in itself is not actually a test (you can see that because it doesn't start with test_), but a method that is used in other tests. The canceled signal is also emitted when the retrieval completed successfully (at least that is what I think, and I understand where your confusion is coming from, it is not that well named in my opinion). That means the cancel button doesn't need to be clicked there, the emission of the cancel signal is an expected effect of clicking btGetCapabilities instead.

The query_server method not being a test in itself also means that all call sites of this method (i.e. the actual tests) need to pass the qtbot fixture to it.

[rohit2p]

i think the failure is because of Fixture Management can you take a look

[matrss]

All of the test failures look like this:

FAILED tests/_test_msui/test_wms_control.py::Test_HSecWMSControlWidget::test_no_server - TypeError: WMSControlWidgetSetup.query_server() missing 1 required positional argument: 'qtbot'

What it means is that all tests that use query_server have to pass along the qtbot fixture that they receive. I.e. the calls to query_server have to be adapted as well, when its signature changes.

Also, there are some minor linting issues from flake8, just click on "Details" of that check to see what they are.

Other than that it is looking good already. Just a minor nitpick: I would like qtbot to be the first argument to query_server, instead of the last. Just to be consistent with other places where this kind of thing is done, e.g. here:

MSS/tests/_test_msui/test_mscolab.py

Line 229 in d4e8d6b

def _login(self, qtbot, emailid="a", password="a"):

[rohit2p]

@matrss the CI failed again can you take a look. this time is it because the missing url arguments are within the context of the test_no_server.

[ReimarBauer]

When you look on the error message

It is pretty verbose, or?

Please have a look on the definition of query_server

[ReimarBauer]

Please look on the definition of the method query_server. I don't know which IDE you use. In pycharm I would do this by

CTRL + click on the query_server word.

you could also debug this, set a breakpoint and go into the line where it is defined.

[rohit2p]

I use PyCharm and have examined the query_server definition. I see it does require a url argument, and I've updated my test_no_server function to provide a mock_url thinking while calling query_server with a dynamic URL might causing the error.

However, the error persists. I'll try debug the code further to pinpoint the issue – any specific debugging tips from your experience would be helpfull
this is what i see why i CTRL+Click:

[rohit2p]

i was trying to debug to find the issue but, unfortunately it seems like the "fork" start method in multiprocessing is not available in my system that why i am encountering the "SKIPPEDe" message because the test requires the "fork" start method. so i looked up i found out i need to switch to 'spawn' so i am trying to figure out how to do that.

[ReimarBauer]

when I do this, I see the declaration a few lines above,

[ReimarBauer]

i was trying to debug to find the issue but, unfortunately it seems like the "fork" start method in multiprocessing is not available in my system that why i am encountering the "SKIPPEDe" message because the test requires the "fork" start method. so i looked up i found out i need to switch to 'spawn' so i am trying to figure out how to do that.

I know, we have a ticket already to change this.

[rohit2p]

when I do this, I see the declaration a few lines above,

can you explain a bit i don't get it @ReimarBauer

[rohit2p]

i was trying to debug to find the issue but, unfortunately it seems like the "fork" start method in multiprocessing is not available in my system that why i am encountering the "SKIPPEDe" message because the test requires the "fork" start method. so i looked up i found out i need to switch to 'spawn' so i am trying to figure out how to do that.

I know, we have a ticket already to change this.

does this means i can't test my code for now on my system

[matrss]

i was trying to debug to find the issue but, unfortunately it seems like the "fork" start method in multiprocessing is not available in my system that why i am encountering the "SKIPPEDe" message because the test requires the "fork" start method. so i looked up i found out i need to switch to 'spawn' so i am trying to figure out how to do that.

I know, we have a ticket already to change this.

does this means i can't test my code for now on my system

Yes, unfortunately the tests that require a running mscolab or mswms server can not be ran on windows at the moment. But the test suite runs in GitHub Actions for every PR, so you can see the test output there.

when I do this, I see the declaration a few lines above,

can you explain a bit i don't get it @ReimarBauer

You have changed the signature of the query_server methods to include an additional parameter. Obviously all code that calls this method now needs to be adapted accordingly and pass in the correct arguments in the correct order.

[rohit2p]

@matrss @ReimarBauer can you take a look does everything look good ? and i don't see wait_until_signal function in utils.py is being used anywhere so can i remove it now. and what about this part in MSS\tests\test_meta.py

 if str(test_file.relative_to(request.config.rootdir)) == "tests/utils.py":
          # Skip tests/utils.py
          # This skip can be removed once wait_until_signal is no longer used
          continue

also i was thinking if removing it is not necessary right now. i could create a new issue out of it and this can be solved through a new PR. It could be a nice Good first issue for maybe newcomers.

[ReimarBauer]

@matrss @ReimarBauer can you take a look does everything look good ? and i don't see wait_until_signal function in utils.py is being used anywhere so can i remove it now. and what about this part in MSS\tests\test_meta.py

 if str(test_file.relative_to(request.config.rootdir)) == "tests/utils.py":
          # Skip tests/utils.py
          # This skip can be removed once wait_until_signal is no longer used
          continue

also i was thinking if removing it is not necessary right now. i could create a new issue out of it and this can be solved through a new PR. It could be a nice Good first issue for maybe newcomers.

I don't think it is a good idea, when we already know the two steps to finish this issue. It may happen that we oversee sometimes something. But in this case it would mean we have to reopen the issue immeaditly because incomplete. We do this also with others when we recognize that it is not done.

[matrss]

and i don't see wait_until_signal function in utils.py is being used anywhere so can i remove it now.

Please remove it.

and what about this part in MSS\tests\test_meta.py

As explained in the comment there this conditional skip can be removed as well, once wait_until_signal is gone.

also i was thinking if removing it is not necessary right now.

I think it is within the scope for this PR to clean up the code that becomes obsolete due to the other changes. I don't want to have lingering dead code remain if it could be cleaned now as well.

[rohit2p]

okey got it thankyou

[rohit2p]

I saw several linting issues in the conf.py and some other files i am curious are there existing known issues or pull requests related to addressing these Flake8 warnings specifically in these files? this issues have been their since January i guess.

[matrss]

Looks good to me now. Thanks for the PR!

[matrss]

I saw several linting issues in the conf.py and some other files i am curious are there existing known issues or pull requests related to addressing these Flake8 warnings specifically in these files? this issues have been their since January i guess.

I assume you mean docs/conf.py, right? Currently we only lint the files under mslib/ and under tests/. There is no specific issue for this yet, but I would prefer if we would lint the entirety of the repository (modulo some auto-generated files). There is a somewhat related issue in #2145, where I wrote a bit about what I would like to have: #2145 (comment). This isn't high up on my list of priorities right now, though.

[rohit2p]

Thanks for clarifying the context.