Create and edit VPC subnets and routers #593
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/oxidecomputer/console-ui-storybook/H9M1ejE3a6dKLyLL1UeEmKUFiuLM |
|
Preview will be deployed at https://console-git-networking-crud.internal.oxide.computer |
| // const name = screen.getByRole('textbox', { name: 'Name' }) | ||
|
|
||
| // this pisses off testing library, "formik change outside of act()". ugh | ||
| // userEvent.type(ipv4, 'new-subnet') |
| SideModal.Section = ({ children }: ChildrenProp) => ( | ||
| <div className="p-8 space-y-6 border-gray-400">{children}</div> | ||
| ) | ||
| SideModal.Section = classed.div`p-8 space-y-6 border-gray-400` |
There was a problem hiding this comment.
using classed gets us the className prop for free
There was a problem hiding this comment.
Yep, much better. I always forget about this.
david-crespo
force-pushed
the
networking-crud
branch
from
4ffc54c
to
5de293f
Compare
david-crespo
changed the title
| screen.getByRole('cell', { name: vpcSubnet2.identity.name }) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
So this test is kind of wild. It takes a few seconds. I'm not sure how brittle it is, it's pretty generic and would be robust to a lot of insignificant things about the page changing. I have liked these kinds of tests in the past because they recreate the kind of manual testing you would do in the browser to make sure happy paths work. But I've never had a whole app full of them, so I don't know how they scale. Curious how you feel about it @zephraph.
There was a problem hiding this comment.
One thing I was thinking while doing the mocks is that MSW would make that part better because when we create the subnet it would actually go into the list of subnets and automatically be returned in the subsequent fetch.
There was a problem hiding this comment.
Yeah, I'm not super keen on the fetchMock usage. It's not the end of the word and having a few here and there are okay, but generally this isn't the best practice. It reminds me of a PR I did for auto. There were a lot of usages of mocks for fs.existsSync and fs.readFile that meant you really had to know what the implementation was doing to write the tests. Changing the order of file reads or using a different method could break things. In that PR, I helped move over to mock-fs which actually mocked out the whole filesystem... meaning all the read/writes were the same, we just had to mock out what exists on the disk. This is ultimately what MSW will give us and is a really strong argument for why it's worth investing the time to build that out.
That said, impact over perfection. I'm fine with this as is b/c it gives us more test coverage than we had. It'd just be nice to come back through and clean it up once we have a better primitive in place.
There was a problem hiding this comment.
Yeah, I'd like to leave this in, as it gives me a lot more to work with when I make MSW work in #571. It will be very cool if we can delete the mock lines and have the rest of test work as-is.
| Default network for the project | ||
| {vpc?.description} |
There was a problem hiding this comment.
Nice, thanks for taking care of this!
| import { classed } from '../util/classed' | ||
| import { classed } from '@oxide/util' |
There was a problem hiding this comment.
Thanks for the cleanup here. We should probably add a lint rule for this stuff at some point.
| @@ -7,7 +7,7 @@ | |||
| "build": "vite build", | |||
| "build-for-nexus": "yarn install && API_URL='' vite build", | |||
| "ci": "yarn tsc && yarn lint && yarn test", | |||
| "test": "jest", | |||
| "test": "DEBUG_PRINT_LIMIT=0 jest", | |||
There was a problem hiding this comment.
Does setting this to 0 mean there is no limit? I didn't see it mentioned in the docs
There was a problem hiding this comment.
No, it completely suppresses the auto HTML dump on testing-library query failure. I basically never find it useful, it just pushes the actual failure message 3 feet upward. Of course this is a consequence of me writing tests that render the entire page instead of one little component at a time. For small components the debug print would be useful.
While messing around with this I discovered when you do want to print the screen for debugging purposes, you can do screen.debug(undefined, <limit_number>). If you want to keep the automatic debug print I can make "tst": "DEBUG_PRINT_LIMIT=0 jest" for myself.
There was a problem hiding this comment.
It's fine to leave it for now. We can revisit if it doesn't work out. I'd rather have one way to do things if we can help it.
To do