Skip to content

Conversation

satyasinha
Copy link

@satyasinha satyasinha commented Nov 6, 2016

No description provided.

@satyasinha satyasinha changed the title COMPASS-226 added string comparison to server version to determine de… COMPASS-226 hiding decimal128 option if server < 3.4 Nov 6, 2016
@satyasinha
Copy link
Author

Wrapped semvar call in a try/catch so test case doesn't complain with empty serverVersion.
Not happy with the single test case to test the existence of serverVersion but it is too tightly bound to fetchFromServer to entangle.

@pzrq
Copy link
Contributor

pzrq commented Nov 8, 2016

@KeyboardTsundoku It looks like the tests need to be updated for the new serverVersion parameter, they pass but now show some extra console spam:

...
  <RangeInput />
    when rendering the default control
      ✓ has label `LOWER BOUND`
      ✓ has placeholder text of `enter lower bound`
    when rendering an upperBound control
      ✓ has label `UPPER BOUND`
      ✓ has placeholder text of `enter upper bound`

  <Rule />
Warning: Failed prop type: Required prop `serverVersion` was not specified in `Rule`.
    in Rule
Warning: Failed prop type: Required prop `serverVersion` was not specified in `RuleCategoryType`.
    in RuleCategoryType (created by Rule)
    in Rule
    in tbody
    in table (created by Constructor)
    in Constructor
Warning: Failed prop type: Required prop `serverVersion` was not specified in `BSONTypeSelector`.
    in BSONTypeSelector (created by RuleCategoryType)
    in RuleCategoryType (created by Rule)
    in form (created by Form)
    in Form (created by Rule)
    in td (created by Rule)
    in tr (created by Rule)
    in Rule
    in tbody
    in table (created by Constructor)
    in Constructor
...

@pzrq
Copy link
Contributor

pzrq commented Nov 8, 2016

@KeyboardTsundoku Also looks like merge conflicts, please rebase, thanks.

@satyasinha satyasinha force-pushed the COMPASS-226-Decimal128-Server-Version branch from 2da946a to c262157 Compare November 8, 2016 06:14
@satyasinha
Copy link
Author

@pzrq should be fixed now

@pzrq pzrq merged commit 447251d into master Nov 8, 2016
@pzrq
Copy link
Contributor

pzrq commented Nov 8, 2016

👍 LGTM merged thanks @KeyboardTsundoku

Minor nitpick is an integration unit test for the different server versions would have been nice, but we aren't enforcing tests on all changes yet from Thomas' Compass Q&A (QA) email.

@pzrq pzrq deleted the COMPASS-226-Decimal128-Server-Version branch November 8, 2016 06:36
pzrq pushed a commit that referenced this pull request Nov 8, 2016
* COMPASS-226 added string comparison to server version to determine decimal hiding

* COMPASS-226 commented out debug statement

* COMPASS-226 testing for travis issues

* COMPASS-226 put server version info in validation store

* COMPASS-226 semvar is struggling with chai tests

* COMPASS-226 slightly cleaner implementation

* COMPASS-226 removed hanging .isRequired in props that add warnings in tests

* Revert unnecessary SortableTable change

See e71d665

* Clean up comments
@imlucas imlucas added this to the 1.5 milestone Nov 14, 2016
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.

3 participants