Skip to content

doc(proposal): add concerns to expectFailure label and/or matcher p…#18

Open
vassudanagunta wants to merge 1 commit intonodejs:mainfrom
vassudanagunta:expect-failure-API-concerns
Open

doc(proposal): add concerns to expectFailure label and/or matcher p…#18
vassudanagunta wants to merge 1 commit intonodejs:mainfrom
vassudanagunta:expect-failure-API-concerns

Conversation

@vassudanagunta
Copy link

Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation.

I think this creates undue maintenance burden. If the user does something like what's outlined here, it will fail. This API isn't that complicated, and I can't think of any precedent in core where we do this much coddling, so IMO it's a case of "RTFM".

Please raise these sorts of things during the initial proposal; there was no revelation that revealed this concern. Proposing a change at the 11th hour creates exactly the churn the proposal process avoids 😕

@vassudanagunta
Copy link
Author

Please raise these sorts of things during the initial proposal; there was no revelation that revealed this concern. Proposing a change at the 11th hour creates exactly the churn the proposal process avoids 😕

In all fairness, I contributed a large amount of effort and time to the evolution of this proposal and the PR, all as a bystander, not as an official reviewer. I don't have the standing of a team member, so hesitated to make even more comments. Perhaps I should not be so shy.

And very often "revelation" only happens when something is coded, when the implications of the design choices become apparent, especially as manifested by test cases. While I'm not in that camp, that's why some people push TDD so hard. In fact, as the "revelations" occurred, for me at least, I asked that test cases be added to make the design implications explicit, so that the test_runner team could take a second look before committing to such as significant change to the API.

Better be safe than sorry.

@JakobJingleheimer
Copy link
Member

JakobJingleheimer commented Feb 25, 2026

Indeed you did contribute a large amount of time and effort!

My comment wasn't intended to scold you, if that's how it was received (and sorry if it did); it was merely to request, indeed, don't be shy, and to highlight the cost of delay. A proposal is exactly the time to throw out crazy ideas, edge-cases to potentially consider, etc 🙂

It also wasn't to dissuade you continuing. If you recall, I asked the implementor to hold tight a couple times (eg nodejs/node#61563 (comment)) during the proposal so we could get all the things ironed out and avoid churn, annnnd why I paused landing the PR to give you time to respond when I realised your unsettled concern.

as a bystander, not as an official reviewer. I don't have the standing of a team member

My perception was that we did not treat you any differently. You technically don't get a vote, but I thought you had the space to speak your mind, and to my knowledge all your concerns were discussed and almost all resolved in a way that you were happy with?

Churn is a very very real problem—quite possibly the biggest reason that we lose implementors. I could see the implementor was getting churn fatigue.


Regarding my block on this PR: it mostly wasn't about the 11th hour¹. It was my vote that I believe the proposal is something we don't need now. But as I mentioned on the implementation PR: I'm open to being wrong, and if I am, I'm happy to revisit.

Also note that it's "changes requested", not "drop it". I said I think it's not needed, not that it's inherently a bad idea.

¹ I blocked instead of just commented because I wanted to prevent idle discussion on this to stymie the implementation at the finish-line. Implementing this proposal wouldn't be a breaking change, so it doesn't need to block the current.

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