test_runner: `t.after` should respect `the first-in-last-out` principle like Golang's defer

[axetroy]

Version

v22.10.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

1. Create a test file

import fs from 'node:fs'
import path from 'node:path'
import test from 'node:test'

test('basic', async (t) => {
    const testDir = path.join(import.meta.dirname, 'logs')

    fs.mkdirSync(testDir, { recursive: true })

    t.after(() => {
        console.log('remove test dir')
        fs.rmdirSync(testDir, { recursive: true })
    })

    fs.writeFileSync(path.join(testDir, 'test.log'), 'hello world!')

    t.after(() => {
        console.log('remove test file')
        fs.unlinkSync(path.join(testDir, 'test.log'))
    })

    // do staff...
})

How often does it reproduce? Is there a required condition?

None

What is the expected behavior? Why is that the expected behavior?

t.after should follow the first-in, last-out principle

According to the above code, the file should be deleted first, then the directory

What do you see instead?

✖ basic (7.4021ms)
  Error: ENOENT: no such file or directory, unlink 'project\folder\logs\test.log'
      at Object.unlinkSync (node:fs:1871:11)
      at TestContext.<anonymous> (file:///path/to/test.test.mjs:19:12)
      at TestHook.runInAsyncScope (node:async_hooks:211:14)
      at TestHook.run (node:internal/test_runner/test:934:25)
      at TestHook.run (node:internal/test_runner/test:1225:18)
      at TestHook.run (node:internal/util:543:20)
      at node:internal/test_runner/test:853:20
      at async Test.runHook (node:internal/test_runner/test:851:7)
      at async after (node:internal/test_runner/test:893:9)
      at async Test.run (node:internal/test_runner/test:942:7) {

Additional information

the first-in-last-out principle is more reasonable and practical. It is useful in many scenarios.

I'm not sure why it was designed in the form of a queue. Is there anything special about it?

[avivkeller]

IMO this code snippet is functioning as intended. The hooks are running in the order they were received.

I don't think we should reverse that order, as IMO it'll only cause confusion and breakages.

-1 from me

[climba03003]

before and after mostly comes with pairs, it is more beneficial to run after in reverse order.
For example, we can create some helper function that automatically cleanup.

async function useDatabase(t) {
  const db = await connect()
  t.before(() => {
    // insert initial data
  })
  t.after(() => {
    // close database connection
  })
  return db
}

test('test', async function(t) {
  const db = await useDatabase(t)
  
  // test operation

  t.after(() => {
    // test cleanup
  })
})

Comparison on the Ecosystem.
tap run in reverse order
jest run in define order but provides jest-jasmine2 to run in reverse order
mocha run in define order
vitest run in reverse order (from testing)

Most of the testing framework using or provides reverse order for after* hooks.

[cjihrig]

I think there is more nuance to this.

Comparison on the Ecosystem.

Most of the testing framework using or provides reverse order for after* hooks.

I'm not sure if this intended to be a comprehensive comparison, but that is not my take away. Jest alone seems to have more npm downloads than all of the "reverse order" runners combined. I'm not saying we should blindly follow Jest, but it is worth pointing out that both approaches are heavily used.

before and after mostly comes with pairs

I'm not sure how accurate this is either. It is definitely one pattern, but may or may not be the most common pattern depending on the application. It's also worth noting that for this to be an issue, you must have multiple after hooks and those hooks must have some dependencies on each other.

Because both approaches are heavily used, I don't think we should break existing node:test users for this.

I do understand the reasoning for wanting the after hooks to work this way though. I wouldn't be opposed to being able to support both approaches (please not another CLI flag for this!). I think the inherited afterEach() hooks might make that tricky though.

[climba03003]

I do understand the reasoning for wanting the after hooks to work this way though. I wouldn't be opposed to being able to support both approaches (please not another CLI flag for this!). I think the inherited afterEach() hooks might make that tricky though.

I would like to mention instead of providing both defined order or reverse order. We might consider what vitest have, it allows users to return a cleanup method inside before (that make more sense in pairs). So, in that case it is the reverse order of the before hook.

It becomes, after* is same the as before and cleanup method is run in reverse order. for example.

async function useDatabase(t) {
  const db = await connect()
  t.before(() => {
    // insert initial data
    console.log(1)
    return () => {
      // cleanup for this particular before hook
      console.log(5)
    }
  })
  return db
}

test('test', async function(t) {
  const db = await useDatabase(t)
  
  // test operation
  console.log(2)

  t.after(() => {
    // test cleanup
    console.log(3)
  })
  t.after(() => {
    // another test cleanup
    console.log(4)
  })
})

[cjihrig]

@climba03003 that sounds like a good proposal to me!

[axetroy]

IMO this code snippet is functioning as intended. The hooks are running in the order they were received.

I don't think we should reverse that order, as IMO it'll only cause confusion and breakages.

-1 from me

Reverse order execution, more human-like cleanup logic, and has been tested in Golang.

If you think this is a BREAKING CHANGE, it's fine to provide an option