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

[STUD-414] Make all tests work with Stardog 7+ #234

Merged
merged 10 commits into from
Oct 13, 2020
Merged

Conversation

jmrog
Copy link
Contributor

@jmrog jmrog commented Oct 5, 2020

No description provided.

@joshhk72
Copy link
Contributor

joshhk72 commented Oct 6, 2020

Maybe super dumb question, but should the tests only be expected to pass for the latest versions of Stardog? Ex: some fail for 7.2.1

Edit: Actually, I just realized this ticket is for 7+ right after reading this since I forgot to read the title.

@joshhk72
Copy link
Contributor

joshhk72 commented Oct 6, 2020

Looks like it's mostly with the JSON-LD stuff, not quite sure what might be the best option to resolve this.

@jmrog
Copy link
Contributor Author

jmrog commented Oct 7, 2020

Looks like it's mostly with the JSON-LD stuff, not quite sure what might be the best option to resolve this.

I think this is mostly okay (for now), but fill free to push back. So far, stardog.js has always been tested against only the latest Stardog. And the failures that you see when you run the tests against earlier versions of Stardog (e.g., 7.2.1 or 6.x.x) are not failures in functionality per se, but are just "failures" because the shapes of Stardog's responses have changed in some cases (I suppose it could be argued that we should therefore make the tests less dependent on the response shapes -- but I'm a little hesitant about that because they currently provide some extra sanity and have caught integration-related issues for us before). Additionally, we're going to release this as part of a major version bump in stardog.js (2.x.x), so no one should be upgrading and expecting that there aren't breaking changes anyway. Combine all of those factors together and I think that just getting things back to a working state with the latest Stardog is a sufficient goal for this PR.

That said, we might want to make a separate push to test stardog.js against earlier versions of Stardog as well, and to make some kind of "contract" regarding what is and isn't supported. I think that can be a separate push, though. But again, feel free to push back. (Also tagging in @anneeb, @cchaffatt, and @lafirey here to see whether they have any thoughts about this.)

@anneeb
Copy link
Contributor

anneeb commented Oct 7, 2020

I think this is mostly okay (for now), but fill free to push back. So far, stardog.js has always been tested against only the latest Stardog. And the failures that you see when you run the tests against earlier versions of Stardog (e.g., 7.2.1 or 6.x.x) are not failures in functionality per se, but are just "failures" because the shapes of Stardog's responses have changed in some cases (I suppose it could be argued that we should therefore make the tests less dependent on the response shapes -- but I'm a little hesitant about that because they currently provide some extra sanity and have caught integration-related issues for us before). Additionally, we're going to release this as part of a major version bump in stardog.js (2.x.x), so no one should be upgrading and expecting that there aren't breaking changes anyway. Combine all of those factors together and I think that just getting things back to a working state with the latest Stardog is a sufficient goal for this PR.

That said, we might want to make a separate push to test stardog.js against earlier versions of Stardog as well, and to make some kind of "contract" regarding what is and isn't supported. I think that can be a separate push, though. But again, feel free to push back. (Also tagging in @anneeb, @cchaffatt, and @lafirey here to see whether they have any thoughts about this.)

I think for the sake of this release, only testing against latest Stardog is okay (this is consistent with the testing expectations we set for Studio currently). I agree that setting up the infrastructure to test against non-latest Stardog should be a separate push, as it will probably require some changes to our snapshots in addition to the CircleCI flows. There's definitely room to improve our documentation, and I think an obvious start would be to explicitly state the compatibility expectations between Stardog.js and Stardog versions, as well as any notes about known deprecation or otherwise breaking changes. I'd lean towards starting to introduce those documentation improvements now though, as they'd pair nicely with the major release.

@joshhk72
Copy link
Contributor

joshhk72 commented Oct 7, 2020

explicitly state the compatibility expectations between Stardog.js and Stardog versions

Second this, if plausible.

@jmrog
Copy link
Contributor Author

jmrog commented Oct 7, 2020

Our summary of things right now exists here: https://github.com/stardog-union/stardog.js#version-details.

Seems like we are/were saying basically that stardog.js 1.x.x works with Stardog 5+, whereas earlier Stardog versions will work with stardog.js 0.x.x. I think we now want to say that stardog.js 2.x.x works with Stardog 7+, stardog.js 1.x.x works with Stardog 5 and 6 (and most of 7, but we won't officially support it there?), and anything earlier than 5 may work with stardog.js 0.x.x. We can probably put this into an easy-to-read table.

Worth noting: I believe that Stardog is officially no longer maintaining versions prior to 7.x.x after the end of this year.

@lafirey
Copy link

lafirey commented Oct 7, 2020

Worth noting: I believe that Stardog is officially no longer maintaining versions prior to 7.x.x after the end of this year.

Correct -- support and maintenance prior to 7 is ending at the end of the year.

The above seems correct to me. As long as we are clear on what version of stardog, stardog.js 2.x.x supports that is fine. I am a little unclear on the before/after json-ld changes in 7.2.1. Would let @jmrog make the final call but I am not opposed to us specifying 7.3 and above for stardog.js 2.x.x. Or at least noting the failures before 7.3

@jmrog
Copy link
Contributor Author

jmrog commented Oct 12, 2020

@joshhk72 I updated this PR to include some of the requested documentation clarifications. Primarily, I added this:

image

Is there anything else blocking the merge here? (Note that this is the merge into v2, not the merge into master, so we still have time to let things brew there.) I'd like to get this in somewhat quickly, if possible, so that other stardog.js work based on v2 isn't held up (or required to branch off of a branch of a branch 😄 ).

Copy link
Contributor

@joshhk72 joshhk72 left a comment

Choose a reason for hiding this comment

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

Some things I was wondering:

  1. Is there a reason we're skipping most of the vg tests?

  2. Should we drop databases after the test? (may not matter since we're not expecting to run these a lot locally?)

it('should optimize an online DB', () =>
// TODO: This case is currently skipped because there is no way in Stardog
// 7.3.x to ensure that the DB is ready for optimization (so, this often
// errors out). The erroring out should be fixed in Stardog 7.4.1.
Copy link
Contributor

Choose a reason for hiding this comment

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

has this been fixed in the 7.4.1 release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it! Updated the PR.

lib/db/main.js Outdated Show resolved Hide resolved
@jmrog
Copy link
Contributor Author

jmrog commented Oct 12, 2020

Some things I was wondering:

  1. Is there a reason we're skipping most of the vg tests?

Yeah. stardog.js tests don't currently set up an actual VG for the tests to work with. There is an open issue for that here: #136

  1. Should we drop databases after the test? (may not matter since we're not expecting to run these a lot locally?)

Which test? The test suites should have an afterAll(dropDatabase(database)) call within them; the ones I checked quickly do have that, but I definitely could have missed one.

@joshhk72
Copy link
Contributor

Ah, I think it leaves one database, but don't think it's too big of a deal for now.

Copy link
Contributor

@joshhk72 joshhk72 left a comment

Choose a reason for hiding this comment

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

LGTM

@jmrog jmrog merged commit f4f77f1 into v2 Oct 13, 2020
@jmrog jmrog deleted the STUD-414/fix-tests-stardog-7 branch October 13, 2020 14:37
jmrog pushed a commit that referenced this pull request Oct 22, 2020
jmrog added a commit that referenced this pull request Feb 22, 2021
* Feature/154 upgrade fetch ponyfill (#155)

* Upgrade fetch ponyfill

* Get tests passing again after upgrading fetch-ponyfill

* Add support, tests, and typings for additionalHandlers on query execute (#157)

* Update package-lock.json

* Fix docs script, update package-lock

* Update CHANGELOG.md for v1.2.1, add tag

* Add release documentation, as well as annoying prepublish alert

* Upgrade typedocs to resolve marked security vulnerability (fix #158)

* 1.2.2

* Update package-lock

* Make additionalHandlers optional

* Add method for retrieving server status info (close #165) (#166)

* Fix package-lock.json for v2

* Add ICV report functionality and fix existing types (#188)

* Update to version 6 and fix CircleCI (#185)

* add full string values for test expectations

* fix tests and update version note in README for stardog 6

* attempt to fix circleci by updating to new jfrog url

* explicitly test graphql explain support (#184)

* Add icv.report and icv.reportInTransaction functionality (close #187)

* Add types for new ICV methods and fix types for existing (close #186)

* Add tests for new ICV methods

* add json query explain support (#191)

* Try to fix dependencies

* Fix tests

* add stored query update (PUT) method

* handle stored query updates when version < 6.2.0

* cleanup poc of server version helper

* instead of getting the version recover from the method not supported error

* cleanup

* cleanup

* code review feedback

* add support for `reasoning` and `description` attributes to stored queries (#196)

* add two endpoints for getting all db properties default values and getting all actual values and upgrade stardog to latest (#198)

* Add ability to import namespaces via string or file (new Stardog endpoints) (#201)

* Add ability to import namespaces via string or file (new Stardog endpoints)

* Use Object.assign instead of spread syntax

* Small tweak

* Docs/type updates and name change

* Docs change

* Update generated docs

* Allow `onResponseStart` to abort further stardog.js processing (#203)

* Allow onResponseStart to abort any further stardog.js processing

* Add test case, plus lint fixes

* Better docs, too

* use params in graphql query (#202)

* Add ability to send properties file on db.create (#205)

* Make old-style namespaces request really request only namespaces (fix #207) (#208)

* Add support for streaming export data to file (and compression) (#212)

* Add support for streaming data export, and for compression

* Add tests for additionalHandlers with exportData

* Add format flag for exportData

* Add ability to import file to VG (for CSV/JSON import) (#213)

* Add support for importing from file to VG (close #209)

* Tests and fixtures

* Fix up tests

* Update README and test name

* Fix types for onResponseStart

* Make input_file come last

* Support updating GraphQL schemas without remove/add (with backwards compat) (#215)

* Add reasoning flag to example

* add db model endpoint (#224)

* Fix db.model type and docs

* add virtual graph list info (#223)

* Add support for database constraint for virtual graphs (#225)

* Handle commented-out prefix lines in queries (fix #228) (#229)

* Add additionalHandlers to graphql.execute (#231)

* Change content-type to use utf-8 (#233)

* [STUD-414] Make all tests work with Stardog 7+ (#234)

* Remove db.copy command and test

* Fix graphql tests

* Fix graphStore tests

* Skip versioning tests for now

* Fix ICV tests

* Fix exportDB tests

* Fix createDB and optimizeDB tests

* Fix query tests

* Update version support info in README

* Update test for Stardog 7.4.1

* Quick test updates

* [STUD-490] add cluster api support (#236)

* [STUD-490] Add cluster api support

* [STUD-490] Removing done callback in spec files

* [STUD-490] Removing unused param and add docs

* [STUD-490] Adding docker-compose cluster test suite

* [STUD-490] feature: add cluster support - address comments

* Test adjustment for Stardog 7.4.2+

* Make tests more flexible

* Remove DB copy and versioning

* Adjust a few tests

* update versions matrix

* Update docs

* [STUD-528] Add Data Sources (#239)

* [STUD-528] add data sources

* rename options to opts

* return res

* [STUD-517] data hub - allow stardog.js to accept query params (#241)

* [STUD-517] feature: data hub - allow stardog.js to accept query params for data sources remove api

* [STUD-517] feature: data hub - pr suggestion, add param to docs, run docs script, append query parameters

* [STUD-517] feature: data hub - update docs, change name string to template

* Add VG online + data source metadata (#240)

* add vg online

* fix docs

* add data source to vg meta

* always send data_source

* no snakes

* [STUD-XXX] Add dataSource.share (#249)

* doc

* share

* [STUD-XXX] Add force option to dataSource.update (#248)

* Add force param to dataSource.update

* Params to request options, don't always include force in body

* Fix doc for vg.mappings

* Update doc for dataSource.update

* [STUD-559] user tests - fix expected res in user tests (#247)

* Add custom changelog generation

* README update

* Update Stardog version info; temporarily disable reasoning tests

* Make some tests rely less on exact serialization from Stardog

Co-authored-by: Brian J. Rubinton <[email protected]>
Co-authored-by: Annee Barrett <[email protected]>
Co-authored-by: Josh Kim <[email protected]>
Co-authored-by: Robert Myers <[email protected]>
Co-authored-by: Carl Chaffatt <[email protected]>
Co-authored-by: Josh Kim <[email protected]>
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.

4 participants