-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
[DEL] [14.0] delivery_cttexpress: non-mocked tests #673
[DEL] [14.0] delivery_cttexpress: non-mocked tests #673
Conversation
…onnection, ruining pipeline for other PRs. Has to be redone using mock.
@thomaspaulb I totally agree on that point. The problem is the author is not understanding why not using real url in tests could cause failing runs in shared repositories... At least, removing tests as you do is not good. We should keep them with mock at least |
Can't we delete them first, or even delete the whole module, and re-merge it back in later? Otherwise we might end up waiting for mocked tests for months, which will be blocking all the other PR |
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.
Of course I don't agree, as they are transient problems, and mocking them is like no having tests, but the tests are adding real value. You have already merged some of the linked PRs, so there's no such problem.
Please don't rant on something that is not really blocking you.
@pedrobaeza Agreed about the ranting aspect! So lets not rant, but I do see conflicting ideas here:
Who decides the policy on this, and how? The PSC's, i guess - is this worth a discussion on the mailinglist perhaps? |
I think it should be the module's maintainer, unless it interferes severely on the repository working, which is not the case. |
That is the subjective part. I tend to think it should not at all. Not relaunching the debate but I give my opinion to the contributor's growing list that are annoyed by. |
As for me, I think I overestimated the "blocking" part, so I'm closing this PR for now - it's not "blocking", just "delaying" things to be merged, and I guess I can live with that. Should it come to a debate later on, I'm rooting for the mock-tests team :-) |
I agree that is not comfortable when this happens, but put it on perspective: when other "online" components like pre-commit download fails (like the infamous If the debate arises in a central topic, not as side one, I'll tell you why mocking doesn't serve most of the time IMO. |
Thanks @pedrobaeza :) |
Remove these tests, which are failing upon remote connection, ruining pipeline for other PRs. Has to be redone using mock.