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

test: Fix is_not_rw_storage #2595

Open
wants to merge 19 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@llukask
Copy link

commented Apr 7, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

@llukask llukask changed the title +Fix is not rw storage test: Fix is_not_rw_storage Apr 7, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Thank you for working on this! Seems like you already solved follow-up problems that turned up after you fixed the function?

@kodebach
Copy link
Contributor

left a comment

Looks good to me (apart from the failing tests)

@llukask

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

@markus2330 Yes to get the tests to run again, I did manually exclude the following plugins from is_not_rw_storage:

  • desktop, c, cpptemplate, specload because they are not really rw-storage plugins
  • mmapstorageand mmapstorage_crc because they can't export to stdout (#2419)
  • tcl, mini, simpleini because they can't deal with values at the root of the exported hierarchy (see my comment on #2423)
  • yambi because it does not seem to export anything at all

llukask added some commits Apr 8, 2019

@llukask

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

I added test files for xerces and excluded yamlcpp because it does not export anything

@sanssecours

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

yambi because it does not seem to export anything at all

YAMBi (Yan LR, YAwn, and YAy PEG) do not include write support. They all use YAML Smith to write data back to a YAML file.

…and excluded yamlcpp because it does not export anything.

Are you sure about that? I tried the following, which seems to work:

kdb set user/tests/key value
kdb export user/tests yamlcpp
#> key: value
kdb export user/tests/key yamlcpp
#> value

.

@llukask

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

@sanssecours

…and excluded yamlcpp because it does not export anything.
Yes, thats not correct.

The problem I encountered with yamlcpp is that like yajl it cannot hold values in directories.

i.e.

kdb set user/tests root
kdb set user/tests/key hello
kdb export user/tests yamlcpp

gives

key: hello

but if you omit setting user/tests/key exporting gives just root

@sanssecours

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

The problem I encountered with yamlcpp is that like yajl it cannot hold values in directories.

Thank you for the clarification. For this purpose YAML CPP requires the Directory Value plugin. Unfortunately kdb export ignores required plugins 😢.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

To get this PR ready, I think we need to exclude these plugins now. But please mark that they should be readded (directly next to the list and in doc/todo/TESTING).

The problem I encountered with yamlcpp is that like yajl it cannot hold values in directories.

yajl should now be able to do so. The PR was merged recently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.