add failing tests for #81

[SamVerschueren]

I added a test case and baseline for #81. It has something to do with this block. If I temporarily change the begin regexp to something like a, this specific test passes.

I noticed PR #129. Not sure if a separate repository is necessary. Just wanted to share this, maybe someone else has an idea.

// @Tyriar

[msftclas]

Hi @SamVerschueren, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[Tyriar]

Failing test 🎆

This is a pretty significant issue for vscode as you can see from the references in #81, hope it gets fixed.

[anubmat]

Hi @SamVerschueren , Since ternary operator does not yet have correct scopes yet, a generated baseline may not be useful. However, if there are some changes you have made leading to correct scopes, then you can simply update this PR by adding your files (YAML-tmlanguage and tmlanguage). It is okay to have another pull request for the same issue. A separate repo is not necessary as only one of them will be merged into this repo eventually. PR #129 is an attempt to give correct scopes to ternary operator. So, in case you just want to add a test case, I can add it to PR #129 .

[SamVerschueren]

@anubmat The baseline is not generated, it is handwritten and that's what the output should be.

Adding failing tests is common practice in open-source world so it's easier for other people (or the maintainers) to fix the issue until the test passes. I did not change anything in the language files (yet). This is just the ease the process for the maintainers to fix this bug.

[DanielRosenwasser]

Hey @SamVerschueren, I get the intent, but adding failing tests makes very little sense to our master branch. While it's often good practice to start with a failing test, this just adds noise to our tests until it the issue is fixed, which makes it harder to realize whether something else regresses.

In other words, it makes sense to base a fix off of your branch, but we wouldn't accept something that causes tests to fail in our master branch.

[sindresorhus]

@DanielRosenwasser I believe the best-practise is to add the failing tests as skipped (or as known failing tests) so they don't fail everything, but are there as a starting point when someone wants to work on fixing the issue. I would strongly recommend finding a way to allow users to submit failing tests (it should of course not be noisy). It's a great way for users to prove an issue is real and lessen the burden on the maintainers to triage issues.

Swift does this: https://github.com/apple/swift/tree/master/validation-test/compiler_crashers

[DanielRosenwasser]

I guess we could have a BROKEN_TESTS branch that people could contribute to and we could cherry pick off of, but I don't see how often this comes up in practice, and I think it would be messier to organize on our end.

Additionally, adding a failing test is usually not the harder part of these fixes. Namely, the fix itself is the difficult part. A branch with the initially failing test and its fixed makes a lot more sense, so feel free to send plenty of those. 😃

I'm all ears for good ideas, but I'm not necessarily seeing something manageable for us right now, so I'm going to close this issue.

[sindresorhus]

I guess we could have a BROKEN_TESTS branch that people could contribute to and we could cherry pick off of

I didn't mean a separate branch. I mean, you mark the test as failing/skipped somehow and ignore those when running tests. Could be as simple as prepending the filename with -skipped. When someone comes along to fix the issue they just remove e.g. filename postfix in the PR to with the fix.

While I might not make sense for this tiny repo, I think it could be useful for the TypeScript project in general.

Additionally, adding a failing test is usually not the harder part of these fixes.

I've run many huge open source projects over the years, and from my experience that's often not true. While it might not apply to this project, in general, many issues are hard to triage and reproduce, but easy to fix when you have some solid tests for it. I personally have to spend most of my time triaging issues.

[DanielRosenwasser]

I mean, you mark the test as failing/skipped somehow and ignore those when running tests. Could be as simple as prepending the filename with -skipped. When someone comes along to fix the issue they just remove e.g. filename postfix in the PR to with the fix.

I guess I don't see why that's functionally better than having the original issue open on GitHub with a minimal repro available. You're asking us to add infrastructure to support adding a test whose only purpose is to act as a TODO. The issue is already a TODO in some sense.

[SamVerschueren]

While I agree that the open issue can be seen as a TODO, a failing test is a good starting point for someone to come along and fix the issue. If the test succeeds, the issue is fixed.

But I believe we can stop discussing as we will probably never agree on this.

[DanielRosenwasser]

Sure, though I hope you don't think I'm closed off to suggestions in the future. I'm definitely willing to consider these ideas.

[SamVerschueren]

No bad feelings. You win some, you loose some ;).