API examples overhaul & roundtrip tests #3204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
teh-cmc
force-pushed
the
cmc/roundtrip_docs
branch
3 times, most recently
from
185f377
to
ed46e8e
Compare
teh-cmc
added
🔨 testing
teh-cmc
force-pushed
the
cmc/roundtrip_docs
branch
from
ed46e8e
to
cc34319
Compare
teh-cmc
changed the title
nikolausWest
reviewed
Sep 5, 2023
nikolausWest
reviewed
Sep 5, 2023
4 tasks
jleibs
reviewed
Sep 5, 2023
| @@ -1097,7 +1144,12 @@ impl RecordingStream { | |||
| /// terms of data durability and ordering. | |||
| /// See [`Self::set_sink`] for more information. | |||
| pub fn connect(&self, addr: std::net::SocketAddr, flush_timeout: Option<std::time::Duration>) { | |||
| self.set_sink(Box::new(crate::log_sink::TcpSink::new(addr, flush_timeout))); | |||
| // NOTE: `forced_sink` is only used for tests, it's ok to unwrap. | |||
There was a problem hiding this comment.
🙈
Really all of our sink APIs should uniformly return a RecordingStreamResult. Having connect actually fail if it can't talk to the server with an option for our current "keep retrying in a background thread" behavior would be a more predictable user experience.
| @@ -1,28 +1,37 @@ | |||
| import rerun as rr | |||
| import rerun.experimental as rr2 | |||
| from rerun.experimental import dt as rrd | |||
There was a problem hiding this comment.
Suggested change
| from rerun.experimental import dt as rrd | |
| from rerun.experimental import dt as rrd |
I think we should avoid rrd as the convention here due to name collision with rrd the file extension.
jleibs
approved these changes
Sep 5, 2023
| {0.f, 1.f, 1.f}, | ||
| }; | ||
| rr_stream.log("segments", rr::LineStrips3D(points)); | ||
| // TODO(#3202): I want to do this! |
There was a problem hiding this comment.
Nit: we don't want to leak these kinds of TODOs into the embedded snippets in the doc-pages.
There was a problem hiding this comment.
I won't do anything about that for now; we'll cleanup during the pre-release doc pass.
#3224 is there to make sure we don't forget!
|
teh-cmc
force-pushed
the
cmc/roundtrip_docs
branch
from
cc34319
to
ba22d22
Compare
teh-cmc
force-pushed
the
cmc/roundtrip_docs
branch
from
ba22d22
to
06f10da
Compare
emilk
reviewed
Sep 6, 2023
teh-cmc
force-pushed
the
cmc/roundtrip_docs
branch
from
b1ccb69
to
42991df
Compare
teh-cmc
force-pushed
the
cmc/roundtrip_docs
branch
from
42991df
to
3d6ebc5
Compare
Size changes
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit by commit
This PR overhauls API examples to make them roundtrippable and checks those roundtrips on CI.
These roundtrips serve two purposes: A) the obvious one: they make sure that using our different SDKs in similar ways actually yield similar data, but more importantly B) they act as regression tests since it is highly unlikely that two or more tests written in different languages fail in the exact same way at the exact same time.
As I've painfully found out during the
MsgSendermigration, a lot of our tests had bitrotten and simply did not work in one way or another anymore (in fact I ended up fixing yet another batch for this PR).This PR should provide the foundations so that this doesn't happen again.
A nice side-effect of this is the introduction of the
_RERUN_TEST_FORCE_SAVEenvironment variable, which forces allRecordingStreams instantiated across all 3 languages to write to an rrd file on disk rather than do whatever they were asked to do.This makes testing things much easier.
What
Checklist