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

Tests for Tutorial #2702

Merged
merged 22 commits into from
Jan 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/Tutorial/Tutorial2StepContextViewOpen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const Tutorial2StepContextViewOpen = () => {
{TUTORIAL_CONTEXT1_PARENT[tutorialChoice]}" or "{TUTORIAL_CONTEXT2_PARENT[tutorialChoice]}" to show it again.
</p>
) : contextViewClosed ? (
<p>Oops, somehow the context view was closed. Select "Relationships".</p>
<p>Oops, somehow the context view was closed. Select "{TUTORIAL_CONTEXT[tutorialChoice]}".</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was a bug I've found while testing

) : (
<>
<p>
Expand Down
1 change: 0 additions & 1 deletion src/components/Tutorial/TutorialNavigationButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ const TutorialNavigationButton = React.forwardRef<
>(({ clickHandler, value, disabled = false, classes }, ref) => (
<a
className={cx(anchorButtonRecipe({ variableWidth: true, smallGapX: true }), classes)}
onClick={clickHandler}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's duplicated by fastClick, which caused a skip of step I've painfully experienced multiple times while testing

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good catch. It doesn't make sense to have both.

{...{ disabled }}
{...fastClick(clickHandler)}
ref={ref}
Expand Down
260 changes: 260 additions & 0 deletions src/components/Tutorial/__tests__/Tutorial.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,260 @@
import { screen } from '@testing-library/dom'
import userEvent from '@testing-library/user-event'
import { act } from 'react'
import createTestApp, { cleanupTestApp, cleanupTestEventHandlers } from '../../../test-helpers/createTestApp'

// as per https://testing-library.com/docs/user-event/options/#advancetimers
// we should avoid using { delay: null }, and use jest.advanceTimersByTime instead
const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime })

/** Gets the last empty thought in the document. Mostly used after `user.keyboard('{Enter}')` to get the new thought. */
const lastEmptyThought = () =>
Array.from(document.querySelectorAll('[data-editable="true"]'))
.filter(it => !it.textContent)
.at(-1)!

describe('Tutorial 1', async () => {
beforeEach(() => createTestApp({ tutorial: true }))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something, but if the tests are all run sequentially on a single app instance, wouldn't this need to be beforeAll instead of beforeEach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really have not much idea how to make it work with one beforeAll, since if I don't create a new test app for each case, I'm left with empty-ish DOM
I think, the whole setup relies on re-creating an isolated testing env for each case, as it's done on every other test suite.

right now it's run with one db/store/state/whatever, but with cleaning up event listeners that keep attaching
but it's not one sequential run per se, though I'd argue it's a lot like reloading page at each step :D

I'm really not sure how to make it work with one beforeAll, really

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your reply. I'd like to dig a little deeper into this. The tests are quite readable right now, but it's not clear what state they share between test blocks.

I really have not much idea how to make it work with one beforeAll, since if I don't create a new test app for each case, I'm left with empty-ish DOM

What do you mean by "empty-ish DOM"?

I think, the whole setup relies on re-creating an isolated testing env for each case, as it's done on every other test suite.

Are you saying that the current implementation recreates an isolated testing environment for each case? Because my understanding is that these tests depend on the current test step being preserved between tests.

right now it's run with one db/store/state/whatever, but with cleaning up event listeners that keep attaching

The event listeners only keep attaching because you remove them and re-attach them each time.

but it's not one sequential run per se, though I'd argue it's a lot like reloading page at each step :D

Is it a sequential run or not? Or some hybrid? I think for clarity we need to stick with one or the other, or more carefully document the unique combination of shared state between test runs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean by "empty-ish DOM"?

html
<body
  data-browser="safari"
  data-color-mode="dark"
  data-device="desktop"
  data-drag-in-progress="false"
  data-native="false"
  data-platform="other"
/>

Are you saying that the current implementation recreates an isolated testing environment for each case? Because my understanding is that these tests depend on the current test step being preserved between tests.

we have everything in db, store etc., but dom is empty if we don't call the createTestApp on each test case
so the current approach, and me attaching and detaching events, is:

  1. we createTestApp, get correct dom state, built from store/db.
  2. we run tests
  3. we remove event listeners created

Just runing a createTestApp once does not work, and it's a problem of our general testing environment.
It's easy to demonstrate if we try to do this:

  1. we createTestApp
  2. we run one test, where we create a thought "first"
  3. we should have "first" thought present in our dom

but it does not work like this at all.
the test suite below gets 2/2 green, but it should not.

describe.only('T_T', () => {
  beforeAll(createTestApp)

  it('we create "first" thought', async () => {
    await user.keyboard('{Enter}')
    await user.type(lastEmptyThought(), 'first')
    await user.keyboard('{Enter}')

    expect(exportContext(store.getState(), HOME_TOKEN)).toEqual(`<ul>
  <li>__ROOT__  
    <ul>
      <li>first</li>
      <li></li>
    </ul>
  </li>
</ul>`)
    expect(screen.getByText('first')).toBeInTheDocument()
  })

  it('we expect to see "first" thought', () => {
    console.log(exportContext(store.getState(), HOME_TOKEN))

    screen.debug(document.querySelector('.content')!) /* <-- prints this
    <body
      data-browser="safari"
      data-color-mode="dark"
      data-device="desktop"
      data-drag-in-progress="false"
      data-native="false"
      data-platform="other"
    />
    */

    expect(() => screen.getByText('first')).toThrow()
  })
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe we should "fix" this, but will it break other tests?

i generally feel like this pr now achieves what we want it to, given the current testing environment
maybe we should create another issue for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I'd love to figure out why the DOM is empty. Without knowing the answer to that, it's hard to know what the right call is here.

However, since all the tests pass and they are quite readable, I'm okay with merging the current implementation so that we have a good checkpoint in the commit history. We can investigate further in a separate issue.

If you could address the small stylistic changes below, then I'd be happy to merge. Thanks!

afterEach(cleanupTestEventHandlers)
afterAll(cleanupTestApp)
describe('step start', () => {
it('we see the welcome text', () => {
expect(screen.getByText('Welcome to your personal thoughtspace.')).toBeInTheDocument()
})

it('we can proceed to first step by clicking Next', async () => {
await user.click(screen.getByText('Next'))
expect(screen.getByText('Hit the Enter key to create a new thought.')).toBeInTheDocument()
})
})

describe('step first thought', () => {
it('we learn how to create a thought', async () => {
await user.keyboard('{Enter}')
expect(screen.getByText('Now type something. Anything will do.')).toBeInTheDocument()
})

it('we create our first thought', async () => {
await user.type(lastEmptyThought(), 'first')
await user.keyboard('{Enter}')
await act(vi.runOnlyPendingTimersAsync)

expect(screen.getByText('Well done!')).toBeInTheDocument()
})
})

it('we create a second thought', async () => {
expect(screen.getByText(/Try adding another thought/)).toBeInTheDocument()
await user.type(lastEmptyThought(), 'second')
await user.keyboard('{Enter}')
await act(vi.runOnlyPendingTimersAsync)

expect(screen.getByText('Now type some text for the new thought.')).toBeInTheDocument()
})

it('we create a third thought', async () => {
await user.type(lastEmptyThought(), 'third')
await user.keyboard('{Enter}')
await act(vi.runOnlyPendingTimersAsync)

expect(screen.getByText(/Hit the Delete key to delete the current blank thought/)).toBeInTheDocument()
})

it('we learn to create nested thoughts', async () => {
/* thoughts:
- first
- second
- third
-
*/
await user.keyboard('{Backspace}')
await user.keyboard('{Control>}{Enter}{/Control}')
await act(vi.runOnlyPendingTimersAsync)

await user.type(lastEmptyThought(), 'child of third')
await act(vi.runOnlyPendingTimersAsync)

expect(screen.getByText(/As you can see, the new thought "child of third" is nested/)).toBeInTheDocument()
})

describe('step autoexpand', async () => {
it('we learn about automatic thought hiding', async () => {
await user.click(screen.getByText('Next'))
expect(screen.getByText(/thoughts are automatically hidden when you click away/)).toBeInTheDocument()
})

it('we hide nested thoughts by clicking away', async () => {
/* thoughts:
- first
- second
- third
- child of third
*/
await user.click(screen.getByText('second'))
expect(screen.getByText(/Notice that "child of third" is hidden now/)).toBeInTheDocument()
})

it('we reveal hidden thoughts by clicking parent', async () => {
await user.click(screen.getAllByText('third').at(-1)!)
expect(screen.getByText('child of third')).toBeInTheDocument()
})
})

describe('step success', async () => {
it('we complete the first tutorial', async () => {
expect(screen.getByText(/Lovely\. You have completed the tutorial/)).toBeInTheDocument()
})

it('we can exit tutorial mode', async () => {
await user.click(screen.getByText('Play on my own'))
await act(vi.runOnlyPendingTimersAsync)
expect(() => screen.getByTestId('tutorial-step')).toThrow('Unable to find an element')
})

it('we can continue to advanced tutorial', async () => {
expect(screen.getByText('Learn more')).toBeInTheDocument()
})
Comment on lines +114 to +116
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a contradiction here: If the user just exited the tutorial, then how could the "Learn more" button be visible?

This seems to rely on the tutorial inadvertently being re-launched on beforeEach, which does not reflect real-world usage. We should make this more explicit, as it's not super obvious what's happening here, and closer to end-user behavior. I recommend one of the following options:

  1. Don't click "Play on my own" at all. Then the test can click "Learn more" just as the user would. (less test coverage)
  2. Click "Play on my own". Then navigate to the Help and start the second Tutorial as the user would. (more work, but more test coverage)

Copy link
Collaborator Author

@snqb snqb Jan 19, 2025

Choose a reason for hiding this comment

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

since I can do it, given the circumstances, I left it as is
at least it tests something that can break easily
and if we'll change our setup to have correct dom each time, this case will fail right away

(test of a test? xd)

})
})

describe('Tutorial 2', async () => {
beforeEach(() => createTestApp({ tutorial: true }))
afterEach(cleanupTestEventHandlers)
afterAll(cleanupTestApp)
it('we learn about context view', async () => {
await user.click(screen.getByText('Learn more'))

expect(screen.getByText(/shows a small number to the right of the thought, for example/)).toBeInTheDocument()
})

it('we choose a project type, one of three options', async () => {
await user.click(screen.getByText('Next'))
expect(screen.getByText(/For this tutorial, choose what kind of content you want to create/)).toBeInTheDocument()
expect(screen.getByText('To-Do List')).toBeInTheDocument()
expect(screen.getByText('Journal Theme')).toBeInTheDocument()
expect(screen.getByText('Book/Podcast Notes')).toBeInTheDocument()
})

it('we start creating a to-do list', async () => {
await user.click(screen.getAllByText('To-Do List').at(-1)!)
expect(screen.getByText(/Excellent choice\. Now create a new thought with the text “Home”/)).toBeInTheDocument()
})

describe('step context 1 - create a "Home" to-do list', () => {
it('we create a "Home" thought', async () => {
await user.keyboard('{Enter}')
await user.type(lastEmptyThought(), 'Home')
await act(vi.runOnlyPendingTimersAsync)
expect(screen.getByText(/Add a thought with the text "To Do"/)).toBeInTheDocument()
})

it('we add a "Home" • "To Do" subthought', async () => {
await user.keyboard('{Control>}{Enter}{/Control}')
await user.type(lastEmptyThought(), 'To Do')
await act(vi.runOnlyPendingTimersAsync)
expect(screen.getByText(/Now add a thought to “To Do”/)).toBeInTheDocument()
})

it('we add a "Home" • "To Do" • "Or to not subthought" sub-subthought', async () => {
await user.keyboard('{Control>}{Enter}{/Control}')
await user.type(lastEmptyThought(), 'Or to not')
await act(vi.runOnlyPendingTimersAsync)

/* thoughts:
- Home
- To Do
- Or to not
*/
expect(screen.getByText(/Nice work!/)).toBeInTheDocument()
})
})

describe('step context 2 - create a "Work" to-do list', () => {
it('we prepare to create another list', async () => {
await user.click(screen.getByText('Next'))
expect(screen.getByText(/Now we are going to create a different "To Do" list./)).toBeInTheDocument()
})

it('we create a Work thought', async () => {
// we created a new thought on 3rd level, clicking "Home" gets us to root
await user.click(screen.getByText('Home'))
await user.keyboard('{Enter}')
await user.type(lastEmptyThought(), 'Work')
await act(vi.runOnlyPendingTimersAsync)

/* thoughts:
- Home
- To Do
- or to not
- Work
*/
expect(screen.getByText(/Now add a thought with the text "To Do"/)).toBeInTheDocument()
})

it('we add another "Work" • "To Do" thought', async () => {
await user.keyboard('{Control>}{Enter}{/Control}')
await user.type(lastEmptyThought(), 'To Do')
expect(screen.getByText('Imagine a new work task. Add it to this “To Do” list.'))
})

it('we see contexts with a superscript', async () => {
expect(screen.getAllByRole('superscript')[0]).toHaveTextContent('2')
expect(screen.getByText(/This means that “To Do” appears in two places/)).toBeInTheDocument()
})

it('we add a "Work" • "To Do" • "Text boss" thought', async () => {
await user.keyboard('{Control>}{Enter}{/Control}')
await user.type(lastEmptyThought(), 'Text boss')
await act(vi.runOnlyPendingTimersAsync)
/* thoughts:
- Home
- To Do
- or to not
- Work
- To Do
- Text boss
*/
expect(screen.getByText('Next')).toBeInTheDocument()
})
})

describe('step context view open', () => {
it('we learn about multiple contexts', async () => {
await user.click(screen.getByText('Next'))
await act(vi.runOnlyPendingTimersAsync)
expect(screen.getByText(/First select "To Do"./)).toBeInTheDocument()
})

it('we select a thought with multiple contexts', async () => {
await user.click(screen.getAllByText('To Do').at(-1)!)
await act(vi.runOnlyPendingTimersAsync)
expect(screen.getByText("Hit Alt + Shift + S to view the current thought's contexts.")).toBeInTheDocument()
})

it('we view contexts of a thought', async () => {
await user.keyboard('{Alt>}{Shift>}S{/Shift}{/Alt}')
await act(vi.runOnlyPendingTimersAsync)
expect(
screen.getByText(/We now see all of the contexts in which "To Do" appears, namely "Home" and "Work"./),
).toBeInTheDocument()
})
})

it('we see real-world context examples', async () => {
await user.click(screen.getByText('Next'))
expect(screen.getByText(/Here are some real-world examples of using contexts in/)).toBeInTheDocument()
})

describe('step success', () => {
it('we complete the advanced tutorial', async () => {
await user.click(screen.getByText('Next'))
expect(screen.getByText(/Congratulations! You have completed Part II of the tutorial./)).toBeInTheDocument()
})

it('we exit the tutorial', async () => {
user.click(screen.getByText('Finish'))
await act(vi.runOnlyPendingTimersAsync)
expect(() => screen.getByTestId('tutorial-step')).toThrow('Unable to find an element')
})
})
})
2 changes: 1 addition & 1 deletion src/components/Tutorial/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ const Tutorial: FC = () => {
>
✕ close tutorial
</a>
<div className={css({ clear: 'both' })}>
<div className={css({ clear: 'both' })} data-testid='tutorial-step'>
<div>
<TransitionGroup>
{tutorialStepComponent ? (
Expand Down
16 changes: 12 additions & 4 deletions src/test-helpers/createTestApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@ import storage from '../util/storage'
let cleanup: Await<ReturnType<typeof initialize>>['cleanup']

/** Set up testing and mock document and window functions. */
const createTestApp = async () => {
const createTestApp = async ({ tutorial }: { tutorial?: boolean } = {}) => {
trevinhofmann marked this conversation as resolved.
Show resolved Hide resolved
await act(async () => {
vi.useFakeTimers({ loopLimit: 100000 })

// calls initEvents, which must be manually cleaned up
const init = await initialize()
cleanup = init.cleanup
Expand All @@ -33,8 +32,8 @@ const createTestApp = async () => {
)

store.dispatch([
// skip tutorial
{ type: 'tutorial', value: false },
// there are cases where we want to show tutorial on test runs, whilst mostly we don't
{ type: 'tutorial', value: !!tutorial },
Copy link
Collaborator

@trevinhofmann trevinhofmann Jan 17, 2025

Choose a reason for hiding this comment

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

Instead of using !!tutorial to handle the undefined cases, it would be nicer to set a default value above so that the tutorial value is always a defined boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI This is a small concession to allow point-free style in beforeEach.

See: #2702 (comment)


// close welcome modal
{ type: 'closeModal' },
Expand Down Expand Up @@ -80,4 +79,13 @@ export const refreshTestApp = async () => {
await act(vi.runOnlyPendingTimersAsync)
}

/** Clear existing event listeners(e.g. keyboard, gestures), but without clearing the app. */
export const cleanupTestEventHandlers = async () => {
raineorshine marked this conversation as resolved.
Show resolved Hide resolved
await act(async () => {
if (cleanup) {
cleanup()
}
})
}

export default createTestApp
Loading