AI-assisted code review with Copilot - April 2023

A year of ghost text and the 22 March announcement

It is 15 April. I have been paying for Copilot since June last year, from the day it left technical preview and received a price tag. Before that, I had access through the waitlist and wrote about it in February 2022. Over those ten months, nothing important has changed in the editor: gray text in front of the cursor, Tab, Esc, and an invoice once a month.

What has changed is everything around it. On 22 March, GitHub announced Copilot X and listed things I do not have at hand today: pull request summaries and descriptions, answers to documentation questions, and help in the terminal. Each of them leads to a form, not an install button. Chat in the editor has been in technical preview behind a waitlist since 22 March, and that is the only sentence I will write about it here because I do not have it in my hands and I am not going to write a tutorial based on someone else’s screenshots.

So I am left with a topic I have had for a long time but never described: what I do with a suggestion Copilot inserts into a test project. That is the entirety of “AI review” for me in April 2023. It is not a bot commenting on a diff on GitHub, but the discipline of accepting suggestions and reading a second time what they leave behind in the file.

For the first two weeks of April, I kept a notebook. I recorded every multiline suggestion I accepted in two test projects and what I did with it afterward. There were 41 entries. This is one person’s notebook from two weeks, not a study, and that is how I will write about it.

I read a suggestion like a colleague’s review

The attitude I have developed fits into one sentence: ghost text is a proposed change, not code that is already mine. When a colleague sends me a patch for a test suite, I read it for the same four things as always. Can the test fail? Where does it get its data? What does it leave behind? Do the names tell the truth about what happens inside? A suggestion goes through the same process.

There is one asymmetry to remember. Behind a colleague’s patch are an intention and a person I can ask why they chose that selector. A suggestion is based on the probability that code in similar places in other repositories looks like this. It sounds like a small detail, but it determines the reading order: with a patch, I first ask “why”; with a suggestion, I first ask “does this even exist?”

A year ago, when writing about test boilerplate, I asked whether ghost text shortens the tedious part of writing a test. The answer was yes, and it has not changed: using statements, braces, a repeated attribute, more [InlineData] entries, an empty page object class with five locators. Copilot still wins there, and I change almost nothing there. This post is about the other half of the same suggestion, the place where you need to know how a specific application behaves.

The application under test is still the same public RealWorld-style demo I have used on the blog since 2021: login, a header with links for the signed-in user, and an article list. This post contains no code from a client project and no logs from a company pipeline, and it will stay that way. I have pinned the versions: NUnit 3.13.3 in the UI project, xUnit 2.4.2 in the helper-layer project, and Playwright runner 1.32.3 since 11 April.

Here is the suggestion I received after writing the test method name. I typed Login_WithValidCredentials_, and a moment later I had a complete test body:

[Test]
public async Task Login_WithValidCredentials_ShowsUserMenu()
{
    await Page.GotoAsync("https://demo.example.com/login");
    await Page.FillAsync("#email", "test@test.com");
    await Page.FillAsync("#password", "Password123!");
    await Page.ClickAsync("button[type=submit]");
    await Page.WaitForTimeoutAsync(2000);
    Assert.That(Page.Url, Is.Not.Null);
}

This compiles, runs, and turns green. It is also useless because it turns green when login is broken too. Page.Url is never null, so the only condition in this test is always satisfied. The method name promises to check the user menu, but the body does not say a single thing about the menu.

The other problems are smaller, but in a regression suite they cost weeks. The address entered in the method bypasses the BaseURL configuration. The login data is a magic constant, and I do not know whether it exists in the environment. WaitForTimeoutAsync hands two seconds over to flakiness: too short on a slow agent and needlessly long locally. The test leaves a session in the database and cleans up nothing afterward.

This is what the suggestion I accepted looks like after review. The minus signs are deletions, the lines I did not keep:

 [Test]
 public async Task Login_WithValidCredentials_ShowsUserMenu()
 {
-    await Page.GotoAsync("https://demo.example.com/login");
-    await Page.FillAsync("#email", "test@test.com");
-    await Page.FillAsync("#password", "Password123!");
-    await Page.ClickAsync("button[type=submit]");
-    await Page.WaitForTimeoutAsync(2000);
-    Assert.That(Page.Url, Is.Not.Null);
+    var user = await _users.CreateAsync();
+    var login = new LoginPage(Page);
+    var header = new HeaderComponent(Page);
+
+    await login.OpenAsync();
+    await login.SignInAsync(user.Email, user.Password);
+
+    await Expect(header.UserMenu).ToBeVisibleAsync();
+    await Expect(header.SignInLink).ToBeHiddenAsync();
 }

I also add two lines that were not in the suggestion at all because Copilot does not know that cleanup exists in this class:

[TearDown]
public async Task CleanupAsync() => await _users.DeleteAsync();

What remains from the suggestion is the method name, the attribute, the async Task signature, and the order of the steps. That is a real saving because it is the part I do not want to write for the hundredth time. Nothing concerning the behavior of my application remains, and that is also a result. You just need to be able to see it instead of pressing Tab three times in a row.

The notebook from two weeks breaks down like this:

These are the reasons behind the 29 corrections and rejections. One suggestion could have two problems, so the total in this table is not the total from the previous one:

I have seen percentages online from last year’s GitHub study about completing a task faster with Copilot. I did not measure that for myself, and I will not repeat that number as my own result. My numbers come from one notebook and say something else: how many times I had to touch an accepted suggestion before I considered it a test.

Where Copilot lies in tests

Three categories from the table recur so regularly that they deserve separate descriptions. I do not call this a tool error. The tool does exactly what it was built to do: produce code that looks plausible at this point in the file.

Selectors and test identifiers. This is the most common and most expensive category because it remains invisible until the first run. Copilot suggests data-testid="user-menu", #login-form, or .article-preview__title, but exactly one of these exists, while the others look like something someone once wrote online for a similar application. I have learned a simple rule: no new selector enters a page object without one run with the page open. When a suggestion lands in a class where locators are already defined and named, its accuracy increases noticeably because the model has something to follow in the same file.

Attributes from another version or another framework. Four cases in two weeks, all of the same type. In the NUnit project, I got [TestFixtureSetUp], a name from NUnit 2 that simply does not exist in the 3.13.x line. In the xUnit project, I got [TestCase] instead of [Theory] with [InlineData], as well as Assert.Equal with a third argument for a message, which xUnit does not accept because it deliberately has no such overloads. This category hurts the least because the compiler rejects it immediately. It is still worth knowing why it appears: I have two frameworks in one solution, as I described in January 2022, while the model sees the file context, not my decision about the split. Incidentally, I am staying on NUnit 3.13.3 and xUnit 2.4.2, and I have no reason to change those pins this quarter.

Steps without verification. Three times I got a test that went through the entire path in the interface and ended on the final action. A sequence of await calls with no assertion at the end is not a test. It is a script that verifies that nothing threw an exception. Sometimes that is enough for a smoke test, but then I want it written in the name and a comment, not inherited from a suggestion. The same applies to assertions that always pass: Is.Not.Null on something that is never null, or Assert.That(list.Count, Is.GreaterThanOrEqualTo(0)). I received both in April.

The conclusion is the same one I wrote down a month ago when using GPT-4 to draft test cases, only one layer lower. The model is good at shape and weak at specifics that depend on a reality it cannot see. The shape of a test, the arrange and act structure, the method name, a repeated attribute - yes. A selector, a field name, a boundary value from a business rule, the version of an attribute in my csproj - no.

Copilot for PRs is an announcement, not my pipeline

Of the 22 March announcement, the part that interests me most concerns pull requests: a change description generated from the diff and suggestions in the description field. I filled in the form, and that is all I have today. It is a waitlist and a technical preview, not a feature I can enable in repository settings, so I will not write about how to configure it or show a screenshot I did not take.

I can, however, write down what I decided before access arrives because this is a process decision and does not depend on whether the feature turns out to be good.

A generated pull request description is a description, not a verdict. A diff summary answers “what changed,” while a review answers “is this safe, and will the tests catch it?” The second question requires knowledge that is not in the diff: what is in production, what went wrong a month ago, and what this client depends on. I also see a risk that is easy to predict: when a smooth summary appears at the top of a pull request, some reviewers will read the summary instead of the diff. This is not the tool’s fault. It is a familiar mechanism, the same one behind clicking “re-run” without reading the log.

I do not intend to plug a model-generated summary into the merge to main as a gating step. My pull request gate has a specific composition, which I described in September when discussing a CI/CD test strategy: the required checks are the build, unit and integration tests with a container, and a selected UI suite, while the nightly run handles the rest. A text summary replaces nothing on that list because it does not execute the code. If it stood alongside them as a required check, I would get one more field to click through and my first pipeline failure on the day someone else’s API had a bad day.

And one thing follows from both points: while I do not have this tool, I am not planning work on the assumption that I will get it soon. For me, April looks like this: a human reviews a pull request, the change author writes the description, and Copilot helps with writing, not approval.

Suggestion review checklist

This is the list I have actually gone through 41 times in April. Three minutes per suggestion, always in the same order.

Can this test fail? This is the first and most important question because a test that cannot fail is worse than no test: it costs CI time and gives a false sense of coverage. I check this in the simplest way possible. I break the expected value or provide the wrong password, and I want to see a red result with a meaningful message. Nine times in two weeks, this attempt showed that the assertion from the suggestion was always true.

Where does the data come from, and where does it go? A magic constant in a test body is a question with no answer: I do not know whether this user exists, who created it, or whether another test is changing it right now. Data comes in through a fixture or helper and is removed after the test. Copilot does not know about my cleanup, so I add [TearDown] and IAsyncLifetime by hand. Here I return to the rule I wrote down when organizing xUnit and NUnit: first determine where state lives, then choose attributes.

Will it survive parallel execution? Suggested code readily reaches for a shared static field, a single user with a fixed login, and a record that is “definitely there.” It passes locally on one thread. Across four jobs, it starts flickering once every few runs, and diagnosing a case like that takes days. I split the suite into jobs a year and a half ago, and the cost of poor isolation has not dropped by a single zloty because no AI tool removes this requirement. It is about test architecture, not writing speed.

Does the name tell the truth? The final question takes seconds and saves future readers. The name Login_WithValidCredentials_ShowsUserMenu paired with an assertion on Page.Url is a lie that someone will read as documentation six months from now. I either change the body to match the name or the name to match the body.

There is also one change of habit, not a checklist item. When a suggestion is longer than two lines, I do not accept it through a series of Tab presses. I accept the whole thing and read the block in the file because it is easy to accept five lines in the editor without reading them and fail to notice that the arrange disappeared in the middle. This one habit accounts for most of my 22 corrections before the commit.

Summary

After two weeks of taking notes, I have a conclusion that fits into one sentence: Copilot shortens the time I spend writing and reading test code, but it does not reduce my responsibility for the merge.

I am breaking that down into three sentences that I want to be able to read a year from now. Of 41 accepted suggestions, 12 remained unchanged, and all were boilerplate, so the promise from March 2022 still holds. I had to correct or reject a suggestion 29 times, and the reasons were always the same four: assertion, data, cleanup, and a selector that does not exist. None of these reasons is new, and I can name all of them, which is why three minutes per suggestion is enough to keep a test that cannot fail out of the suite.

What does not follow from this? It does not mean I have AI review on pull requests because Copilot for PRs has been behind a waitlist since 22 March, and all I have is a completed form. It does not mean I have stopped reading diffs. It does not mean that anyone has become unnecessary because a tool that gave me a selector not present in the application six times in April is not a candidate for a reviewer.

A human remains the owner of the merge, and that is not false modesty. I put my name to the test suite I leave behind in the repository, and you can only put your name to something you have read. A suggestion shortens the path to the first version of a test, but it does not shorten the path from that first version to the one I let into main by even a minute. The latter has always been more expensive, and it still is in April 2023.

In a month or two, I will know whether the things announced on 22 March change anything in this picture. When I get access, I will describe them on my own stack and with my own numbers. Until then, I keep the same practice as today: gray text as a proposal, a checklist as a filter, and a red result as proof that a test checks anything at all.