Skip to content
This repository was archived by the owner on Feb 28, 2020. It is now read-only.

fix: update health routes to be codable #472

Merged
merged 10 commits into from
Mar 29, 2018

Conversation

KyeMaloy97
Copy link
Contributor

Updated the health routes file to leverage codable routing, no regression as the behavior is the same as before, it returns two items if theres an issue and one if there isn't.

@codecov-io
Copy link

codecov-io commented Mar 8, 2018

Codecov Report

Merging #472 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #472   +/-   ##
========================================
  Coverage    90.61%   90.61%           
========================================
  Files            9        9           
  Lines         1343     1343           
========================================
  Hits          1217     1217           
  Misses         126      126

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 c2907a1...39ceac4. Read the comment docs.

@christiancompton christiancompton changed the title Update health routes to be codable fix: update health routes to be codable Mar 9, 2018
if let getResult = String(data: data, encoding: String.Encoding.utf8) {
XCTAssertEqual(statusCode, 200)
XCTAssertTrue(getResult.contains("UP"))
XCTAssertTrue(getResult.contains("2017") || getResult.contains("2018") || getResult.contains("2019"))
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a more dynamic timestamp check here? This will be a pain to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to dynamically get the current year and make a check that its the same as returned in the test.

let yearString = String(describing: calendar.component(.year, from: date))
XCTAssertTrue(getResult.contains(yearString)
} else {
XCTFail("Return value from /health was nil, or was not UP, or had no timestamp.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this error message doesn't give correct information about the error.
It is better to put the errors inside the asserts such as :
XCTAssertEqual(statusCode, 200, "Error")
And set this XCTFail to "The conversion to String(..) failed" something along those lines which is what the if is checking for.

let date = Date()
let calendar = Calendar.current
let yearString = String(describing: calendar.component(.year, from: date))
XCTAssertTrue(getResult.contains(yearString)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there is a parentheses missing here

@christiancompton
Copy link
Member

@KyeMaloy97 Can you address the concerns above?

@christiancompton christiancompton merged commit 21cfea5 into IBM-Swift:develop Mar 29, 2018
christiancompton pushed a commit that referenced this pull request Apr 3, 2018
* fix(package): update generator-ibm-cloud-enablement to version 0.12.0

Closes #484

* Adding xunit-file and updating package.json to run mocha

* chore(devops-insights): Updating commit message

* fix: update health routes to be codable (#472)

* fix: updated health routes to use codable

* chore(release): 4.9.2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants