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

Modify query tests to fix failures (v4) #1811

Closed
wants to merge 3 commits into from
Closed

Modify query tests to fix failures (v4) #1811

wants to merge 3 commits into from

Conversation

markdirish
Copy link

Several basic-querying.test.js tests fail when running with loopback-connector-ibmi. Explanations can be found in issue #1809. This PR fixes the tests on v3 (LTS) branch.

Part of #1809

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@markdirish Thank you for the contribution! Would it make more sense to do the string conversion in loopback-connector-ibmi instead of in the test? Otherwise the code changes LGTM.

// every name beginning with a capital letter, and the test will fail.
let compareString = 'xyz';
if (db.adapter.name === 'ibmi') { // i.e. "if EBCDIC encoding"
compareString = 'XYZ';
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make more sense to do the conversion in loopback-connector-ibmi?

@dhmlau dhmlau added the community-contribution Patches contributed by community label Dec 18, 2019
@@ -653,7 +663,7 @@ describe('basic-querying', function() {
});
});

describe('geo queries', function() {
bdd.describeIf(connectorCapabilities.geoPoint, 'geo queries', function() {
Copy link
Member

Choose a reason for hiding this comment

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

The comment #1810 (comment) is applicable to this pull request too. Let's discuss the matter in the other PR first, and then apply the conclusions here.

Hmm, I believe we are intentionally testing geo queries for all connectors, including those that don't provide native GeoPoint & near query support. IIRC, when the connector does not implement near queries, juggler will perform the search in memory.

Take db2 and dashdb for an example. According to the check on the line 52 above, these connectors don't support GeoPoint type, yet they still pass all describe('geo queries') tests.

Let's find a way how to allow the ibmi connector pass these test too.

@stale
Copy link

stale bot commented Mar 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 6, 2020
@stale
Copy link

stale bot commented Mar 20, 2020

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Patches contributed by community stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants