Skip to content

WIP: base for #4883#4937

Open
aksonov wants to merge 2 commits into
masterfrom
4883
Open

WIP: base for #4883#4937
aksonov wants to merge 2 commits into
masterfrom
4883

Conversation

@aksonov

@aksonov aksonov commented Mar 18, 2020

Copy link
Copy Markdown
Contributor

@southerneer Maybe you have idea why original formValidation test 'first name ends with space'' works well but the same firstName doesn't throw error from my refactored copy?

P. S. Btw, I found that rn-chat tests are not run and fail now

@aksonov aksonov requested a review from southerneer March 18, 2020 15:46
Comment thread src/model/Profile.ts
// eslint-disable-next-line
const isAlphabet = '^\\pL[ 0-9`\'"\u0060\u00B4\u2018\u2019\u201C\u201D\\pL]*[\\pL0-9]?$'

const profileConstraints = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please consider moving isAlphabet and profileConstraints to the bottom of Profile.ts. It won't have any technical impact, and will leave the more important/referenced code near the top of the file.

@southerneer

Copy link
Copy Markdown

The problem is that the original test was giving a false positive.

  it('first name ends with space', async () => {
    try {
      await validateProfile({firstName: 'Eric '})
    } catch (err) {
      expect(err).toStrictEqual({
        firstName: ['First name can only contain alphabet characters'],
      })
    }
  })

Inside the try block I should have expected it to throw an error like this...

expect(await validateProfile({firstName: 'Eric '})).toThrow()

So while the correct error may have been thrown at some point in the past, it was lost in future revisions but the test kept on saying everything was ok.

@bengtan

bengtan commented Mar 19, 2020

Copy link
Copy Markdown
Contributor

I found that rn-chat tests are not run and fail now

I dug into this.

In PR 4187 ('upgrade to RN0.61') there was this comment:

#4187 (comment)

I'm going to deactivate jest snapshot tests for now, create a ticket, and reactivate later when they're fixed.

followed by commit f4c7c43 which removed ... npm run jestCI from the CLI yarn test

PR 4187 was merged and into 4.27.0.

I'm guessing that when @southerneer disabled the jest snapshot tests (*.test.tsx) by removing ... npm run jestCI, he didn't realise it also disabled HomeStore.test.ts and formValidation.test.ts?

So ... maybe consider doing the following?:

  1. Re-enable HomeStore.test.ts and formValidation.test.ts again. An easy way would be to copy them into the wocky tests.
  2. Re-visit jest snapshot tests again (optional)? (Probably as a separate ticket/PR.)
  3. Review the scripts in package.json as the naming/structure of yarn jestCI, yarn jestWockyCI etc. is a bit out of date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants