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

#140 added tests for req #142

Merged
merged 2 commits into from Oct 11, 2020
Merged

#140 added tests for req #142

merged 2 commits into from Oct 11, 2020

Conversation

MaurizioPz
Copy link
Contributor

This PR adds the test listed on #140

but the test 'Should ignore charset' is ignored because right now it doesn't pass. I may have misinterpreted the desired test, but as of right now https://github.com/jshttp/media-typer seems to be using a regex that doesn't handle charset

@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #142 into master will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   68.43%   68.60%   +0.17%     
==========================================
  Files          49       49              
  Lines        1752     1752              
  Branches      441      441              
==========================================
+ Hits         1199     1202       +3     
+ Misses        552      549       -3     
  Partials        1        1              
Impacted Files Coverage Δ
packages/req/src/index.ts 89.65% <0.00%> (+10.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d4a9f0...6d1a71c. Read the comment docs.

@talentlessguy
Copy link
Member

@MaurizioPz by "should ignore charset" for req.is I meant that if you send a Content-Type that includes a charset, it must ignore it and check for the MIME type only

@MaurizioPz
Copy link
Contributor Author

Well than there's a bug right now, because it sounds like you are expecting this test to pass and it doesn't

    it('should ignore charset', async () => {
      const app = runServer((req, res) => {
        expect(reqIs(req)('text/html')).toBe('text/html')
        res.end()
      })
      await makeFetch(app)('/', {
        headers: {
          'Content-Type': 'text/html; charset=UTF-8',
        },
      })
      expect.assertions(1)
    })

@BRA1L0R
Copy link
Contributor

BRA1L0R commented Oct 10, 2020

@MaurizioPz i've investigated on the issue and pointed it out in #150. You can work on it if you want as it is very easy to fix but also relatively important.

@talentlessguy
Copy link
Member

@MaurizioPz could you pls fix the conflict so we can merge this?

# Conflicts:
#	__tests__/modules/req.test.ts
@talentlessguy
Copy link
Member

nice, now it looks good

merging

@talentlessguy talentlessguy merged commit dd95e4b into tinyhttp:master Oct 11, 2020
@talentlessguy
Copy link
Member

@all-contributors add @MaurizioPz for test

@allcontributors
Copy link
Contributor

@talentlessguy

I've put up a pull request to add @MaurizioPz! 🎉

@talentlessguy talentlessguy mentioned this pull request Oct 14, 2020
17 tasks
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.

None yet

4 participants