search

rnpgp / ruby-rnp

Ruby bindings for librnp
https://www.rnpgp.org
MIT License
7 stars 3 forks source link

Update spec to correctly handle userid validity checks. #69

Closed ni4 closed 3 years ago

ni4 commented 3 years ago

This PR updates spec to be up-to-date with behavior changes from https://github.com/rnpgp/rnp/pull/1337 As it updates tests only, I don't think we should add some checks for rnp versions and so on. However, if there is a better approach all suggestions are appreciated.

codecov[bot] commented 3 years ago

Codecov Report

Merging #69 (ba3fd68) into master (fdee8c9) will decrease coverage by 0.02%. The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   98.94%   98.91%   -0.03%     
==========================================
  Files          41       41              
  Lines        3325     3327       +2     
==========================================
+ Hits         3290     3291       +1     
- Misses         35       36       +1
Impacted Files Coverage Δ
lib/rnp/misc.rb 100.00% <ø> (ø)
spec/key/properties/4BE147BB22DF1E60_spec.rb 98.61% <66.66%> (-1.39%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fdee8c9...ba3fd68. Read the comment docs.

ni4 commented 3 years ago

Ping @ronaldtse @dewyatt : could we merge this despite the CI failures? Or, what should be a workflow when some changes in rnp require tests here to be updated?

dewyatt commented 3 years ago

Ping @ronaldtse @dewyatt : could we merge this despite the CI failures? Or, what should be a workflow when some changes in rnp require tests here to be updated?

I'll take a closer look later but at a glance the test looks to be specific to the version of RNP and should be made compatible with all supported versions. As-is it looks like it would break support for all other versions of RNP.

ni4 commented 3 years ago

I'll take a closer look later but at a glance the test looks to be specific to the version of RNP and should be made compatible with all supported versions. As-is it looks like it would break support for all other versions of RNP.

Thanks, @dewyatt . I updated the PR with version checks so it should work with every RNP version. However tests seems to behave weird, let's see whether some restarts would help.

dewyatt commented 3 years ago

Sorry I'm dragging my feet on this but I'll get back to it in the next few days here

ni4 commented 3 years ago

@dewyatt Looks like we now have at least two issues:

I'll update the PR with latest timestamp check, but what would be a correct way to fix Botan's build? Use GCC 9.0? Report error to their tracker? Both :) ?

dewyatt commented 3 years ago

@dewyatt Looks like we now have at least two issues:

  • latest Botan 3.0 alpha uses flag -Wextra-semi, which is available only since GCC 9.0, while 7.5 is used.
  • there were some commits to the RNP master, so "LibRnp::rnp_version_commit_timestamp >= 1604156729" is not good check anymore.

I'll update the PR with latest timestamp check, but what would be a correct way to fix Botan's build? Use GCC 9.0? Report error to their tracker? Both :) ?

It's probably fine to just test against the latest release of botan and not master, which would take care of that issue.

Yeah typically what I would do here is merge the corresponding rnp PR first (with CI broken -- test manually) and then use that commit timestamp. Also the version thing is something I would rather see in FEATURES (which is maybe better named quirks) in lib/misc.rb.

ni4 commented 3 years ago

@dewyatt Thanks, updated PR. Does it look good now (except that we'll need to update the timestamp once https://github.com/rnpgp/rnp/pull/1337 is merged)?

ni4 commented 3 years ago

@ronaldtse All tests except codecov are passing now, so let's merge this? P.S. Btw, where did we lose codecov for the rnp? CC @dewyatt

ronaldtse commented 3 years ago

P.S. Btw, where did we lose codecov for the rnp?

@ni4 not sure why it's gone -- it's still enabled in Settings. Maybe we need this to make it work? https://github.com/codecov/codecov-action