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

Implement label.get_labels, core.enable_plugin and core.disable_plugin #40

Merged
merged 15 commits into from Dec 26, 2021

Conversation

relvacode
Copy link
Contributor

Implement the label.get_labels RPC method.

I'm happy to add a test if you can guide me how to construct the hex payload for the mock client

@gdm85
Copy link
Owner

gdm85 commented Dec 20, 2021

Thanks for your PR @relvacode!

See https://github.com/gdm85/go-libdeluge/pull/39/files for example: first I add the method in integration_test.go (if it works for both v1 and v2, or in the corresponding files if method is exclusive to a specific version), then the printServerResponse call in such method will print out the hexadecimal bytes that can be used to mock it.

So there are always actually 2 tests: an integration test which talks to the real Deluge daemon (both v1 and v2) and a mocked test that uses the synthetic bytes payload.

@relvacode
Copy link
Contributor Author

I think I've added all that's needed @gdm85, I've also added the method to delugecli as well. Let me know if I should change something

@relvacode
Copy link
Contributor Author

The GitHub build is failing in my fork. Presumably this is because the Label plugin isn't enabled with the default Deluge configuration.

Would need some mechanism for the integration test to create core.conf with at least

"enabled_plugins": ["Label"]

delugecli/cli.go Outdated Show resolved Hide resolved
plugins.go Outdated Show resolved Hide resolved
@gdm85
Copy link
Owner

gdm85 commented Dec 22, 2021

The GitHub build is failing in my fork. Presumably this is because the Label plugin isn't enabled with the default Deluge configuration.

Would need some mechanism for the integration test to create core.conf with at least

"enabled_plugins": ["Label"]

That should be relatively easy to accomplish with some changes to scripts/deluge-install.sh, for example for v2 the core.conf should contain:

{
    "file": 1,
    "format": 1
}{
    "enabled_plugins": ["Label"]
}

However I think that a better approach would be to implement core.enable_plugin and core.disable_plugin and call them before/after (defer) the execution of the test, otherwise there is a risk for the integration tests to "assume" that the labels plugin is always enabled.

Would you like to add those as well? Otherwise I can do it.

@relvacode
Copy link
Contributor Author

Yeah that sounds useful. I can do that

@relvacode relvacode changed the title Implement label.get_labels Implement label.get_labels, core.enable_plugin and core.disable_plugin Dec 23, 2021
@relvacode
Copy link
Contributor Author

Okay that should be it @gdm85 the build passes for me.

Note I had to change https://github.com/relvacode/go-libdeluge/blob/feature/label.get_labels/integration/integration_test.go#L212 to use the most recent RPC call (as the plugin test will have >0 RPC calls before printServerResponse is called)

methods.go Outdated Show resolved Hide resolved
methods.go Outdated Show resolved Hide resolved
integration/integration_plugins_test.go Outdated Show resolved Hide resolved
integration/integration_plugins_test.go Outdated Show resolved Hide resolved
@gdm85
Copy link
Owner

gdm85 commented Dec 23, 2021

Thanks @relvacode; there is something odd there happening in the defer, I left some comments.

@gdm85 gdm85 merged commit 8481c1e into gdm85:master Dec 26, 2021
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.

None yet

2 participants