Avoiding LGTM PR Cultures

Introduction

Making a code change when using a distributed version control system (DVCS) like Git is usually done by packaging a change on a branch as a “pull request” (PR) which indicates the author would like the project to “pull” the change into it.

This was, and is, a key part of open source projects as it allows outside contributors to contribute to a project in a controlled way, however many internal software development teams also work in this fashion as there are many benefits of this approach over committing directly to a shared branch or trunk.

I’ve seen the pull request approach have a positive impact on software quality since pull requests facilitate discussion through peer reviews and allow running of automated tests against every commit and change that is proposed to be merged into the default branch.

taken lgtm.jpg

What is a LGTM PR culture?

I’ve also seen some negative behaviours emerge when moving to pull request based development which I’ll call a LGTM PR culture.

LGTM is a common acronym found in peer reviews of pull requests which means “Looks Good To Me”, and I’ve seen teams let unsuitable changes through with LGTM comments without doing solid peer reviews and testing.

How do you know if you have a LGTM PR culture?

One way to “test” your peer review process is by creating PRs and leaving a subtle bug or something not quite right that you know about in the PR. When it gets reviewed do you get a LGTM? I did this recently and whilst the PR didn’t even do what it was meant to do I received a LGTM 😕

2dsag2

How can you move away from a LGTM PR culture?

It’s tempting to just tell everyone to do better peer reviews but it’s not that easy!

I’ve found there’s some steps that an author of a pull request can do to facilitate better pull request reviews and lead towards a better culture.

1. Make pull requests as small as possible:

The smaller the pull request the more likely you’ll get specific and broad feedback on it – and you can then iterate on that feedback. A 500 line change is daunting to review and will lead to more LGTMs. For larger refactorings where there’ll be lots of lines changed, you can start with a small focussed change and get lots of review and discussion, and once the refactoring is established with a smaller example you can easily apply that feedback to a broader impact PR that won’t need as much feedback as you’ve established the new pattern previously.

2. Review your own pull request

Review your own work. This works best if you do something else then come back to it with a fresh mind. Anything you’re unsure about or doesn’t look right leave it as a comment on your own PR to encourage other reviewers to look closely at those areas also.

3. Include clear instructions for reviewing and testing your pull request

A list of test steps is good as well as asking for what type of feedback you’d like – you can explicitly ask reviewers something like “please leave a comment after your review listing what you tested and what areas of the code you reviewed.” This discourages shortcuts and LGTMs.

4. Test your peer review process – see above.

Conclusion

Developing software using pull requests can mean much higher quality code and less technical debt due to the feedback on peer reviews that accompany pull requests. As an author you can take steps to ensuring pull requests are easy to review and encourage a culture of effective peer reviews.

npm ci

I recently discovered npm ci which you can use instead of npm install when running a node project on continuous integration (CI) system and want to install your npm dependencies. It does this in a more lightweight, more CI friendly way.

If you use npm test to run your tests, this can be shortened to npm t (much like npm i is npm install), and therefore you can run npm cit to install dependencies and run tests in CI.

Running Mocha e2e Tests in Parallel

I recently highlighted the importance of e2e test design. Once you have well designed e2e tests you can start running them in parallel.

There are a couple of approaches to scaling your tests out to be run in parallel:

  1. Running the tests in multiple machine processes;
  2. Running the tests across multiple (virtual) machines;

These aren’t mutually exclusive, you can run tests in parallel processes across multiple virtual machines – we do this at Automattic – each test run happens across two virtual machines, Docker containers on CircleCI, each of which runs six processes for either desktop or mobile responsive browser sizes depending on the container.

I have found running tests in multiple processes gives best bang for buck since you don’t need additional machines (most build systems charge based on container utilisation) and you’ll benefit from parallel runs on a local machine.

We write our e2e tests in WebDriverJs and use Mocha for our test framework. We currently use a tool called Magellan to run our e2e tests in separate processes (workers), however the tool is losing Mocha support and therefore we need to look at alternatives.

I have found that mocha-parallel-tests seems like the best replacement – it’s a drop in replacement runner for mocha tests which splits test specification files across processes available on your machine you’re executing your tests on – you can also specify a maximum limit of processes as the command line argument --max-parallel

There is another parallel test runner for mocha: mocha.parallel – but this requires updating all your specs to use a different API to allow the parallelisation to work. I like mocha-parallel-tests as it just works.

I’ve updated my webdriver-js-demo project to use mocha-parallel-tests – feel free to check it out here.

Running e2e Tests in Parallel

One of the best ways to speed up your end-to-end (e2e) tests is to start running them in parallel.

The main issue I see that prevents teams from fully using parallelism for their e2e tests is lack of test design. Without adequately designed e2e tests – which have been designed to be run in parallel – parallelism can introduce non-deterministic and inconsistent test results – leading to frustration and low-confidence in the e2e tests.

This is often the case when teams go about directly converting manual test cases into automated e2e tests – instead of approaching e2e test automation with a specific end-to-end design focus.

Say you had a manual test case for inviting someone to view your WordPress blog:

  1. Enter the email address of the person you’d like to follow your site
  2. Check the list shows a pending invite
  3. Check your email inbox shows a pending invite
  4. Open the email, follow the link and sign up for a new account

When you’re manually testing this in sequence it’s easy to follow – but as soon as you start running this in parallel, across different builds, and with other tests things will most likely start failing.

Why? The test isn’t specific enough – you may have multiple pending invites – so how do you know which one is which? You can only invite someone once, how do you generate new invite emails? You may receive multiple emails at any one time – which one is which? And more.

With appropriate e2e test design you can write the e2e test to be consistent when run regardless of parallelism:

  1. Email addresses are uniquely generated for each test run using a GUID and either a test email API like Mailosaur or Gmail plus addressing; and
  2. The pending email list has a data attribute on each invite clearly identifying which email the invite is for and this is used to verify pending email status; and
  3. The inbox is filtered by the expected GUID, and only those emails are used. Etc.

Once you have good e2e test design in place you’re able to look at how to speed up e2e test execution using parallelism. I’ll cover how to do this in my next blog post.

Bailing with Mocha e2e Tests

At Automattic we use Mocha to write our end-to-end (e2e) automated tests in JavaScript/Node.js. One issue with Mocha is that it’s not really a tool suited to writing e2e tests where one test step can rely on a previous test step – for example our sign up process is a series of pages/steps which rely on the previous step passing. Mocha is primarily a unit testing tool and it’s bad practice for one unit test to depend on another, so that is why Mocha doesn’t support this.

A more simplified example of this is shown in my webdriver-js-demo project:

describe( 'Ralph Says', function() {
	this.timeout( mochaTimeoutMS );

	before( async function() {
		const builder = new webdriver.Builder().withCapabilities( webdriver.Capabilities.chrome() );
		driver = await builder.build();
	} );

	it( 'Visit the page', async function() {
		page = await RalphSaysPage.Visit( driver );
	} );

	it( 'shows a quote container', async function() {
		assert( await page.quoteContainerPresent(), 'Quote container not displayed' );
	} );

	it( 'shows a non-empty quote', async function() {
		assert.notEqual( await page.quoteTextDisplayed(), '', 'Quote is empty' );
	} );

	afterEach( async function() { await driver.manage().deleteAllCookies(); } );

	after( async function() { await driver.quit(); } );
} );

Continue reading “Bailing with Mocha e2e Tests”

Using async/await with WebDriverJs

We’ve been using WebDriverJs for a number of years and the control flow promise manager that it offers to make writing WebDriverJs commands in a synchronous blocking way a bit easier, particularly when using promises.

The problem with the promise manager is that it is hard to understand its magic as sometimes it just works, and other times it was very confusing and not very predictable. It was also harder to develop and support by the Selenium project so it’s being deprecated later this year.

Fortunately recent versions of Node.js support asynchronous functions and use of the await command which makes writing WebDriverJs tests so much easier and understandable.

I’ve recently updated my WebDriverJs demo project to use async/await so I’ll use that project as examples to explain what is involved.

WebDriverJs would allow you to write consecutive statements like this without worrying about waiting for each statement to finish – note the use of test.it instead of the usual mocha it function:

test.it( 'can wait for an element to appear', function() {
	const page = new WebDriverJsDemoPage( driver, true );
	page.waitForChildElementToAppear();
	page.childElementPresent().then( ( present ) => {
		assert( present, 'The child element is not present' );
	} );
} );

When you were waiting on the return value from a promise you could use a .then function to wait for the value as shown above.

This is quite a simple example and this could get complicated pretty quickly.

Since the promise manager is being removed, we need to update our tests so they continue to execute in the correct order. We can make the test function asynchronous by adding the async prefix, remove the test. prefix on the it block, and add await statements every time we expect a statement to finish before continuing:

it( 'can wait for an element to appear', async function() {
	const page = new WebDriverJsDemoPage( driver, true );
	await page.waitForChildElementToAppear();
	assert( await page.childElementPresent(), 'The child element is not present' );
} );

I personally find this much easier to read and understand, less ‘magic’, but the one bit that stands out is visiting the page and creating the new page object. The code in the constructor for this page, and other pages, is asynchronous as well, however we can’t have an async constructor!

export default class BasePage {
	constructor( driver, expectedElementSelector, visit = false, url = null ) {
		this.explicitWaitMS = config.get( 'explicitWaitMS' );
		this.driver = driver;
		this.expectedElementSelector = expectedElementSelector;
		this.url = url;

		if ( visit ) this.driver.get( this.url );

		this.driver.wait( until.elementLocated( this.expectedElementSelector ), this.explicitWaitMS );
	}
}

How we can get around this is to define a static async function that acts as a constructor and returns our new page object for us.

So, our BasePage now looks like:

export default class BasePage {
	constructor( driver, expectedElementSelector, url = null ) {
		this.explicitWaitMS = config.get( 'explicitWaitMS' );
		this.driver = driver;
		this.expectedElementSelector = expectedElementSelector;
		this.url = url;
	}

	static async Expect( driver ) {
		const page = new this( driver );
		await page.driver.wait( until.elementLocated( page.expectedElementSelector ), page.explicitWaitMS );
		return page;
	}

	static async Visit( driver, url ) {
		const page = new this( driver, url );
		if ( ! page.url ) {
			throw new Error( `URL is required to visit the ${ page.name }` );
		}
		await page.driver.get( page.url );
		await page.driver.wait( until.elementLocated( page.expectedElementSelector ), page.explicitWaitMS );
		return page;
	}
}

In our Expect and Visit functions we call new this( driver ) which creates an instance of the child class which suits our purposes. So, this means our spec now looks like:

it( 'can wait for an element to appear', async function() {
	const page = await WebDriverJsDemoPage.Visit( driver );
	await page.waitForChildElementToAppear();
	assert( await page.childElementPresent(), 'The child element is not present' );
} );

which means we can await visiting and creating our page objects and we don’t have any asynchronous code in our constructors for our pages. Nice.

Once we’re ready to not use the promise manager we can set SELENIUM_PROMISE_MANAGER to 0 and it won’t use it any more.

Summary

The promise manager is being removed in WebDriverJs but using await in async functions is a much nicer solution anyway, so now is the time to make the move, what are you awaiting for? 😊

Check out the full demo code at https://github.com/alisterscott/webdriver-js-demo

→ The Rise of the Software Verifier

View story at Medium.com

I found this article rather interesting. I’m still not sure if some of it is satire, forgive me if I misinterpreted it.

“DevOps has become so sophisticated that there is little fear of bugs. DevOps teams can now deploy in increments, monitor logs for misbehavior, and push a new version with fixes so fast that only a few users are ever affected. Modern software development has squeezed the testers out of testing.

Features are more important than quality when teams are moving fast. Frankly, when a modern tester finds a crashing bug with strange, goofy, or non-sensical input, the development team often just groans and sets the priority of the bug to the level at which it will never actually get fixed. The art of testing and finding obscure bugs just isn’t appreciated anymore. As a result, testers today spend 80% of their time verifying basic software features, and only 20% of their time trying to break the software.”

The author doesn’t say where the 80:20 figures came from, but the testers I have worked with for the last five years have spent zero time on manual regression testing verification and most of their time actually testing the software we were developing. How did we achieve this? Not by splitting our team into testers and verifiers as the author suggests:

What to do about all this? The fix is a pretty obvious one. Software Verification is important. Software Testing is important. But, they are very different jobs. We should just call things what they are, and split the field in two. Software testers who spend their day trying to break large pieces of important software, and software verifiers, who spend their time making sure apps behave as expected day-to-day should be recognized for what they are actually doing. The world needs to see the rise of the “Software Verifier”.

We did this by focussing on automating enough tests that we were confident to release our software frequently being confident we weren’t introducing major regressions. This wasn’t 100% test coverage, it was just enough test coverage to avoid human verification. We obviously spent effort maintaining these tests, but that’s a whole team effort and it freed up a lot of time to spend the rest of our time testing the software and looking for real life bugs using human techniques.

Another thing I noted about the article was the use of the graph to show decreasing interest in software testing:

But even their interest is Software Testing fading fast…

https://ssl.gstatic.com/trends_nrtr/1386_RC02/embed_loader.js

trends.embed.renderExploreWidget(“TIMESERIES”, {“comparisonItem”:[{“keyword”:”software testing”,”geo”:””,”time”:”2004-01-01 2018-05-03″}],”category”:0,”property”:””}, {“exploreQuery”:”date=all&q=software%20testing”,”guestPath”:”https://trends.google.com:443/trends/embed/”});

 
This also applies to software in general, perhaps even more dramatically:

https://ssl.gstatic.com/trends_nrtr/1386_RC02/embed_loader.js

trends.embed.renderExploreWidget(“TIMESERIES”, {“comparisonItem”:[{“keyword”:”software”,”geo”:””,”time”:”2004-01-01 2018-05-03″}],”category”:0,”property”:””}, {“exploreQuery”:”date=all&q=software”,”guestPath”:”https://trends.google.com:443/trends/embed/”});

I don’t think there’s a decreasing interest in software testing, or software, but rather these have become more commonplace and more commoditised, so people need to search for these less.