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

Feat(test): giving some love to tests #47

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

rgoupil
Copy link
Contributor

@rgoupil rgoupil commented Oct 5, 2022

Description

It's quite common to look at tests when docs are not enough, especially for specific use case.
In its current state, the code is not showing using the framework in the most elegant way and we still have a few URL dependencies.
This PR aim to improve all those points by giving some love to the tests <3

It also introduce a single breaking change which is to change the default value for the port parameter of DanetApplication.listen() from 3000 to 0. Most respectable frameworks (Express, NestJS, Oak, etc.) will use 0 if no port is provided, why not Danet!


Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have run deno lint AND deno fmt AND deno task test and got no errors.
  • I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING.md
  • I have checked to ensure there aren't other open Pull Requests for the same update/change?
  • I have performed a self-review of my own code
  • I have made corresponding changes needed to the documentation

src/app.ts Outdated Show resolved Hide resolved
@Sorikairox
Copy link
Collaborator

Nice PR ! Not quite sure about the random port, so it would be cool to put it back to a fixed number (maybe not 3000, but 4242 or idk)

@codecov-commenter
Copy link

Codecov Report

Merging #47 (a49eb99) into main (123e508) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #47   +/-   ##
=======================================
  Coverage   84.60%   84.60%           
=======================================
  Files          26       26           
  Lines        1104     1104           
  Branches       90       90           
=======================================
  Hits          934      934           
  Misses        168      168           
  Partials        2        2           
Impacted Files Coverage Δ
src/deps.ts 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Sorikairox Sorikairox merged commit 56f760e into Savory:main Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants