Skip to content
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

R: Add snapshot tests for indentation #2577

Merged
merged 7 commits into from
Apr 2, 2024
Merged

R: Add snapshot tests for indentation #2577

merged 7 commits into from
Apr 2, 2024

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Mar 29, 2024

This adds snapshot tests for indentation issues described in:

Closes #1694.

I initially tried to import the indentation test cases from ESS (https://github.com/emacs-ess/ESS/blob/master/test/styles/RStudio-.R). However those tests assume you can do something like the reindent lines action.

  • Reindent lines doesn't work here because it only uses regular indentation rules without on-enter rules, which isn't consistent with what happens when the user types Enter.

  • Pasting with auto-indent doesn't work either because it doesn't match the behaviour the user would see when typing the code manually. For instance, typing a closing delimiter like ) triggers a "matching indent", see

    if (!isDoingComposition && this._isAutoIndentType(config, model, selections)) {
    const commands: Array<ICommand | null> = [];
    let autoIndentFails = false;
    for (let i = 0, len = selections.length; i < len; i++) {
    commands[i] = this._runAutoIndentType(config, model, selections[i], ch);
    if (!commands[i]) {
    autoIndentFails = true;
    break;
    }
    }
    if (!autoIndentFails) {
    return new EditOperationResult(EditOperationType.TypingOther, commands, {
    shouldPushStackElementBefore: true,
    shouldPushStackElementAfter: false,
    });
    }
    }
    . This special dedenting behaviour doesn't happen on paste.

  • Typing the whole file in a loop one character at a time doesn't work either, because typing an opening delimiter like { auto-closes it, which corrupts the snapshots. Disabling auto-closing doesn't help because without the closing delimiters, we can't reproduce some of the regexp rules sensitive to those delimiters.

After struggling a while I gave up on this approach. I think we need to test only one action per snapshot case. This is what is implemented in the indentation-cases.R (inputs) and indentation-snapshots.R (outputs) files. These contain a list of fenced snapshots with a special marker indicating the cursor position. The snapshots are inserted in an editor and a newline is typed at the cursor. The result is pushed to the snapshot files.

The infrastructure for this is based on the Typescript tests for indentation that @juliasilge discovered: https://github.com/posit-dev/positron/blob/main/extensions/typescript-language-features/src/test/unit/onEnter.test.ts#L36C1-L36C1. These are disabled because of intermittent failures on Windows so we'll have to watch out for that.

I've added a VS Code workspace inside the test folder to hold overridden settings. That's a bit of a heavier setup that I'd like but I don't think it's possible to create a virtual context where we could override settings for unit tests?

In terms of workflow these snapshots are in line with the way testthat handled snapshots pre edition 3. The snapshots are always regeneratedv If they don't match this is an error, but rerunning the test will no longer fail as the snapshots have been updated. The differences should be inspected with git and either accepted if the new output is correct, or discarded if it's not.

@@ -0,0 +1,5 @@
Launch tests by running this from the repository root:
Copy link
Collaborator

@petetronic petetronic Mar 29, 2024

Choose a reason for hiding this comment

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

I like having a README

import * as fs from 'fs';
import { CURSOR, type, withFileEditor } from './editor-utils';

const snapshotsFolder = `${__dirname}/../../src/test/snapshots`;
Copy link
Member

Choose a reason for hiding this comment

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

I recently eliminated these sorts of inline ad hoc paths in favor of using EXTENSION_ROOT_DIR everywhere possible inside the extension:

#2550

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, done!

Copy link
Contributor

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

This is really exciting to get some of these behaviors under test. 🎉

A couple of minor questions/thoughts:

  • Let's make the change Jenny suggested for the path.
  • I looked through a few extensions and didn't see this kind of snapshot or "golden" test used elsewhere. For our information, did you find any? Not because I think we should change anything, but because I'd like to see what kinds of practices people use.

@lionel- lionel- force-pushed the feature/indent-infra branch from fd23d44 to 5032416 Compare April 2, 2024 13:18
@lionel-
Copy link
Contributor Author

lionel- commented Apr 2, 2024

I looked through a few extensions and didn't see this kind of snapshot or "golden" test used elsewhere. For our information, did you find any? Not because I think we should change anything, but because I'd like to see what kinds of practices people use.

I did not find any, they seem to prefer writing snippets inline in the typescript code. I think the snapshot approach implemented here has less friction for adding cases though.

@lionel- lionel- merged commit 7a3d14b into main Apr 2, 2024
1 check passed
@lionel- lionel- deleted the feature/indent-infra branch April 2, 2024 13:49
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.

R: Add testing for indentation and onEnterRules behaviors
4 participants