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

[router] remove duplicate context mocking function #26651

Merged
merged 6 commits into from
Jan 29, 2024

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Jan 24, 2024

Why

There are two functions used to mocking a context in our tests, with createMockContextModule only being used by the configFromFs.

Unfortunately the tests using configFromFs are subtlety incorrect. context.keys() are relative paths (with a ./ prefix). Without this, getRoutes() creates a two tier state, with an empty name route for the top level { name: '' }. createMockContextModule does not enforce this, so while technically the getPathsFromState tests are correct, what they are testing and what our expectation is are different.

You can verify that these tests were incorrect by making this change on main

- const config = configFromFs(['_layout.tsx', 'bar.tsx', 'index.tsx']);
+ const config = configFromFs(['./_layout.tsx', './bar.tsx', './index.tsx']);

expect(getStateFromPath<object>(path, config)).toEqual({
-  routes: [{ name: '', state: { routes: [{ name: 'bar', path: '/bar' }] } }],
+  routes: [{ name: 'bar', path: '/bar' }],
});

I've updated the getPathsFromState to use a new helper getMockState that utilises the getMockContext function.

A future advantage of this refactor is the ability to test the configs being generated with unstable_settings

const config = getMockConfig({
  page: {
    unstable_settings: {},
    default: () => null
  }
})

Test Plan

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 24, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jan 24, 2024
@@ -45,26 +45,30 @@ describe('baseUrl', () => {
process.env.EXPO_BASE_URL = '/expo/prefix';

const path = '/expo/prefix/bar';
const config = configFromFs(['_layout.tsx', 'bar.tsx', 'index.tsx']);
const config = getMockConfig({
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if this function also accepted an array of strings for brevity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done & updated the documentation

@marklawlor marklawlor force-pushed the marklawlor/router/remove-configFromFs branch from bb5bc29 to 8c1d540 Compare January 29, 2024 00:54
@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Jan 29, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jan 29, 2024
docs/pages/router/reference/testing.mdx Outdated Show resolved Hide resolved
@marklawlor marklawlor force-pushed the marklawlor/router/remove-configFromFs branch from 9d825d0 to 8e526ff Compare January 29, 2024 12:23
@marklawlor marklawlor merged commit aacfbf0 into main Jan 29, 2024
13 checks passed
@marklawlor marklawlor deleted the marklawlor/router/remove-configFromFs branch January 29, 2024 20:41
marklawlor added a commit that referenced this pull request Feb 28, 2024
# Why

There are two functions used to mocking a context in our tests, with
`createMockContextModule` only being used by the `configFromFs`.

Unfortunately the tests using `configFromFs` are subtlety incorrect.
`context.keys()` are relative paths (with a `./` prefix). Without this,
`getRoutes()` creates a two tier state, with an empty name route for the
top level `{ name: '' }`. `createMockContextModule` does not enforce
this, so while technically the `getPathsFromState` tests are correct,
what they are testing and what our expectation is are different.

You can verify that these tests were incorrect by making this change on
`main`

```diff
- const config = configFromFs(['_layout.tsx', 'bar.tsx', 'index.tsx']);
+ const config = configFromFs(['./_layout.tsx', './bar.tsx', './index.tsx']);

expect(getStateFromPath<object>(path, config)).toEqual({
-  routes: [{ name: '', state: { routes: [{ name: 'bar', path: '/bar' }] } }],
+  routes: [{ name: 'bar', path: '/bar' }],
});
```

I've updated the `getPathsFromState` to use a new helper `getMockState`
that utilises the `getMockContext` function.

A future advantage of this refactor is the ability to test the configs
being generated with `unstable_settings`

```tsx
const config = getMockConfig({
  page: {
    unstable_settings: {},
    default: () => null
  }
})
```




# Test Plan



<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants