test: dynamic imports #1918
test: dynamic imports #1918
Conversation
🏁 Performance SummaryCommit:
📊 View Container Resources in the Workflow SummaryLatency History🔵 Min • 🟢 Avg • 🔴 Max %%{init: {"xyChart": {"width": 500, "height": 200}}}%%
xychart-beta
title "TCP"
x-axis "PR Number" 1599 --> 1938
y-axis "Latency (ms)"
line "Min" [0.315, 0.429, 0.308, 0.349, 0.397, 0.259, 0.317, 0.333, 0.266, 0.278, 0.322, 0.300, 0.301, 0.302, 0.263, 0.338, 0.288, 0.308, 0.318, 0.313, 0.358, 0.317, 0.274, 0.293, 0.278, 0.312, 0.308, 0.280, 0.296, 0.292, 0.296, 0.296, 0.437, 0.265, 0.444, 0.396, 0.323, 0.305, 0.305, 0.293, 0.333, 0.322, 0.370, 0.273, 0.338, 0.275, 0.318, 0.322, 0.405, 0.278, 0.325, 0.297, 0.275, 0.297, 0.280, 0.277, 0.407, 0.304, 0.329, 0.319, 0.372, 0.289, 0.317, 0.262, 0.298, 0.376, 0.331, 0.257, 0.286, 0.299, 0.267, 0.530, 0.277, 0.240, 0.266, 0.424, 0.235, 0.261, 0.364, 0.422, 0.274, 0.297, 0.313, 0.291, 0.317, 0.356, 0.274, 0.222, 0.290, 0.253, 0.319, 0.268, 0.303, 0.246, 0.304, 0.316, 0.217, 0.252, 0.431, 0.247, 0.291, 0.267, 0.297, 0.392, 0.267, 0.335, 0.447, 0.289, 0.251, 0.361, 0.400, 0.393, 0.323, 0.267, 0.357, 0.284, 0.266, 0.319, 0.284, 0.299, 0.340, 0.323, 0.239, 0.300, 0.323, 0.339, 0.274, 0.272, 0.342, 0.594, 0.341, 0.254, 0.492, 0.404, 0.499, 0.418, 0.565, 0.343, 0.537, 0.486, 0.487, 0.497, 0.532, 0.582, 0.544, 0.482, 0.502, 0.464, 0.463, 0.510, 0.496, 0.424, 0.363, 0.437, 0.505, 0.484, 0.449, 0.453, 0.648, 0.572, 0.494, 0.498, 0.466, 0.528, 0.407, 0.503, 0.424, 0.473, 0.380, 0.535, 0.423, 0.493, 0.465, 0.439, 0.428, 0.467, 0.492, 0.461, 0.458, 0.469, 0.513, 0.532, 0.414, 0.379, 0.462, 0.462, 0.458, 0.466, 0.450, 0.432, 0.457, 0.424, 0.502, 0.420, 0.515, 0.448, 0.438, 0.503, 0.542, 0.392, 0.516, 0.465, 0.457, 0.470, 0.529, 0.501, 0.409, 0.485, 0.377, 0.392, 0.555, 0.369, 0.320, 0.528, 0.448, 0.465, 0.487, 0.433, 0.549, 0.468, 0.323, 0.373, 0.341, 0.519, 0.602, 0.372, 0.567, 0.389, 0.426, 0.430, 0.573, 0.425, 0.463]
line "Avg" [1.042, 1.134, 1.015, 1.035, 1.177, 0.899, 1.040, 1.085, 0.860, 1.015, 1.020, 0.945, 0.986, 1.074, 0.899, 0.975, 0.958, 1.027, 0.959, 1.047, 1.103, 1.046, 0.908, 0.882, 0.896, 1.064, 1.011, 0.957, 0.991, 0.923, 0.867, 0.959, 1.126, 0.910, 1.125, 1.085, 0.983, 1.049, 1.053, 1.056, 1.080, 0.989, 1.137, 1.007, 1.062, 0.874, 1.065, 1.137, 1.321, 0.852, 1.080, 0.991, 1.003, 1.017, 0.971, 1.068, 1.026, 1.054, 1.100, 1.054, 1.087, 0.961, 0.886, 0.878, 1.028, 1.247, 1.023, 0.887, 1.015, 0.952, 0.875, 1.716, 1.003, 0.997, 0.982, 1.178, 0.804, 0.839, 1.092, 1.110, 0.850, 1.026, 0.996, 0.959, 1.046, 1.083, 0.907, 0.852, 0.933, 0.953, 1.103, 0.932, 1.033, 0.974, 0.901, 1.042, 0.807, 0.838, 1.146, 1.110, 0.967, 0.981, 0.902, 1.065, 0.980, 1.053, 1.187, 0.955, 0.857, 1.106, 1.137, 1.130, 1.215, 1.003, 1.135, 0.933, 0.939, 0.997, 0.910, 0.949, 1.081, 1.009, 0.860, 1.047, 0.958, 0.999, 1.007, 0.870, 1.585, 1.622, 1.666, 0.923, 1.652, 1.383, 1.573, 1.415, 1.903, 1.399, 1.464, 1.422, 1.637, 1.594, 1.636, 1.531, 1.644, 1.674, 1.504, 1.422, 1.780, 1.681, 1.519, 1.252, 1.365, 1.539, 1.592, 1.591, 1.708, 1.391, 1.909, 1.760, 1.483, 1.432, 1.549, 1.699, 1.448, 1.533, 1.597, 1.353, 1.221, 1.833, 1.523, 1.475, 1.355, 1.485, 1.827, 1.522, 1.602, 1.429, 1.607, 1.528, 1.480, 1.731, 1.834, 1.308, 1.853, 1.399, 1.696, 1.611, 1.365, 1.297, 1.469, 1.585, 1.775, 1.401, 1.732, 1.582, 1.506, 1.410, 1.592, 1.339, 1.465, 1.328, 1.384, 1.545, 1.439, 1.657, 1.779, 1.433, 1.428, 2.023, 1.749, 1.435, 1.495, 1.427, 1.421, 1.487, 1.588, 1.243, 1.510, 1.542, 1.793, 1.222, 1.373, 1.607, 1.515, 1.575, 1.485, 1.583, 1.543, 1.916, 1.770, 1.390, 1.738]
line "Max" [2.298, 2.513, 2.647, 2.370, 2.486, 2.258, 2.316, 2.407, 2.065, 3.120, 2.328, 2.193, 2.359, 2.398, 2.082, 2.241, 2.339, 2.187, 2.267, 2.268, 2.480, 2.591, 2.231, 1.950, 2.493, 2.596, 2.270, 2.198, 2.587, 2.539, 1.811, 2.355, 2.373, 2.516, 2.368, 2.412, 2.166, 2.482, 2.401, 2.505, 2.396, 2.211, 2.686, 2.876, 2.527, 2.543, 2.441, 2.495, 3.568, 2.073, 2.354, 2.257, 2.217, 2.562, 2.383, 3.300, 2.337, 2.394, 2.521, 2.316, 2.490, 2.352, 2.218, 2.233, 2.267, 2.836, 2.352, 2.316, 2.371, 2.306, 1.972, 3.568, 2.457, 2.517, 2.295, 2.685, 1.995, 1.988, 2.495, 2.309, 2.154, 2.482, 2.433, 2.387, 2.387, 2.335, 2.411, 2.018, 2.145, 2.515, 2.441, 2.129, 2.243, 2.354, 2.225, 2.359, 2.232, 2.120, 3.212, 3.323, 2.256, 2.348, 2.230, 2.484, 2.177, 2.245, 2.542, 2.492, 2.067, 2.443, 2.400, 2.524, 2.732, 2.678, 2.575, 2.320, 2.945, 2.775, 2.175, 2.225, 2.827, 2.412, 2.462, 2.354, 2.303, 2.117, 2.404, 2.228, 3.568, 3.422, 3.568, 2.286, 3.568, 3.568, 3.568, 3.112, 3.568, 3.568, 3.349, 3.582, 3.568, 3.568, 3.568, 3.211, 3.568, 3.568, 3.401, 3.568, 3.568, 3.568, 3.568, 2.844, 3.133, 3.568, 3.568, 3.568, 3.568, 3.568, 3.568, 6.031, 3.568, 3.255, 3.568, 3.568, 3.540, 3.694, 3.568, 3.391, 2.740, 3.568, 3.568, 3.568, 3.214, 3.373, 3.568, 3.568, 3.568, 3.161, 3.568, 3.568, 4.087, 3.568, 3.568, 3.568, 3.568, 3.399, 3.569, 3.568, 3.367, 2.849, 3.604, 3.568, 3.568, 3.568, 3.286, 3.568, 3.366, 2.914, 3.014, 2.823, 3.114, 2.886, 3.056, 3.270, 3.034, 3.211, 3.230, 3.017, 2.801, 3.696, 3.013, 2.960, 2.848, 3.011, 3.047, 2.859, 41.554, 2.632, 3.259, 2.890, 41.585, 2.622, 2.918, 3.285, 3.071, 2.941, 2.944, 40.959, 2.960, 41.617, 41.997, 2.977, 10.590]
%%{init: {"xyChart": {"width": 500, "height": 200}}}%%
xychart-beta
title "QUIC"
x-axis "PR Number" 1685 --> 1938
y-axis "Latency (ms)"
line "Min" [0.000, 0.659, 0.000, 0.325, 0.229, 0.000, 0.000, 0.208, 0.223, 0.000, 0.000, 0.000, 0.000, 0.271, 0.203, 0.186, 0.000, 0.219, 0.254, 0.000, 0.000, 0.338, 0.000, 0.000, 0.207, 0.000, 0.221, 0.229, 0.000, 0.322, 0.000, 0.205, 0.000, 0.368, 0.213, 0.000, 0.187, 0.263, 0.179, 0.000, 0.323, 0.000, 0.000, 0.000, 0.000, 0.169, 0.197, 0.000, 0.306, 0.000, 0.000, 0.000, 0.448, 0.000, 0.242, 0.423, 0.000, 0.357, 0.392, 0.000, 0.000, 0.404, 0.364, 0.000, 0.000, 0.424, 0.000, 0.000, 0.000, 0.359, 0.449, 0.296, 0.364, 0.286, 0.000, 0.396, 0.000, 0.336, 0.369, 0.381, 0.370, 0.420, 0.451, 0.407, 0.000, 0.483, 0.583, 0.000, 0.588, 0.528, 0.572, 0.220, 0.368, 0.507, 0.549, 0.387, 0.559, 0.571, 0.609, 0.637, 0.441, 0.491, 0.609, 0.636, 0.586, 0.512, 0.412, 0.535, 0.548, 0.582, 0.592, 0.390, 0.571, 0.660, 0.582, 0.497, 0.401, 0.621, 0.589, 0.441, 0.651, 0.563, 0.471, 0.596, 0.600, 0.622, 0.537, 0.688, 0.491, 0.526, 0.646, 0.470, 0.518, 0.477, 0.515, 0.418, 0.571, 0.555, 0.634, 0.629, 0.537, 0.596, 0.559, 0.446, 0.398, 0.483, 0.691, 0.664, 0.565, 0.620, 0.487, 0.550, 0.514, 0.507, 0.473, 0.452]
line "Avg" [1.069, 1.707, 0.259, 1.020, 0.856, 0.725, 0.602, 0.893, 0.875, 0.661, 0.557, 0.731, 0.796, 0.949, 0.726, 0.849, 0.761, 0.718, 0.939, 0.550, 0.561, 0.943, 0.832, 0.807, 0.867, 0.633, 1.098, 0.735, 0.608, 1.008, 0.530, 0.823, 0.822, 1.001, 0.919, 0.789, 0.785, 0.862, 0.741, 0.384, 0.935, 0.539, 0.664, 0.689, 0.829, 0.677, 0.918, 0.625, 0.880, 0.554, 0.506, 0.520, 1.456, 0.838, 0.873, 1.195, 0.824, 1.267, 1.176, 0.885, 0.661, 1.160, 1.118, 0.990, 1.148, 1.215, 1.187, 1.033, 1.098, 1.268, 1.200, 1.277, 1.181, 1.244, 0.863, 1.411, 0.948, 1.204, 1.203, 1.323, 1.341, 1.366, 1.242, 1.218, 1.116, 1.650, 1.700, 1.018, 1.666, 1.618, 1.699, 1.034, 1.254, 1.552, 1.605, 1.509, 1.613, 1.720, 1.646, 1.781, 1.639, 1.600, 1.780, 1.830, 1.749, 1.636, 1.531, 1.607, 1.667, 1.720, 1.716, 1.414, 1.555, 1.760, 1.690, 1.590, 1.385, 1.644, 1.727, 1.561, 1.827, 1.725, 1.688, 1.626, 1.608, 1.643, 1.608, 1.725, 1.557, 1.668, 1.690, 1.509, 1.661, 1.564, 1.614, 1.552, 1.697, 1.667, 1.601, 1.579, 1.525, 1.757, 1.675, 1.481, 1.363, 1.638, 1.828, 1.823, 1.622, 1.751, 1.513, 1.718, 1.633, 1.607, 1.535, 1.474]
line "Max" [2.783, 6.605, 0.821, 1.944, 1.917, 1.681, 1.573, 1.837, 1.857, 1.782, 1.274, 1.783, 2.676, 2.006, 1.943, 2.179, 1.918, 1.755, 1.959, 1.371, 1.455, 1.958, 2.546, 1.978, 1.997, 1.535, 2.434, 1.934, 1.541, 2.256, 1.587, 1.879, 1.830, 2.144, 1.966, 1.840, 1.831, 1.849, 1.714, 1.796, 2.375, 1.343, 1.580, 1.815, 2.349, 1.672, 1.771, 1.586, 1.984, 1.678, 1.404, 1.476, 3.783, 2.333, 2.056, 2.873, 2.073, 2.809, 2.498, 2.435, 1.792, 2.426, 2.214, 2.181, 2.954, 2.497, 2.517, 2.223, 2.517, 2.560, 2.569, 2.510, 2.509, 2.615, 2.074, 4.512, 2.230, 2.542, 2.512, 3.394, 2.811, 3.227, 2.571, 2.708, 2.375, 3.946, 4.238, 2.346, 4.349, 4.111, 4.722, 2.563, 2.672, 3.922, 4.296, 3.873, 4.155, 4.365, 4.428, 4.471, 4.182, 4.108, 4.544, 4.572, 4.385, 3.944, 4.006, 4.238, 4.119, 4.910, 4.201, 4.154, 3.851, 4.208, 3.972, 4.180, 4.207, 3.611, 4.619, 5.194, 3.998, 3.792, 3.652, 3.594, 3.831, 3.703, 3.748, 3.740, 3.660, 3.663, 3.740, 3.650, 3.712, 3.473, 3.397, 3.419, 3.935, 3.699, 3.564, 3.483, 3.390, 3.787, 3.808, 3.437, 3.336, 3.721, 4.142, 4.136, 3.732, 3.796, 3.601, 3.910, 3.803, 3.551, 3.510, 3.413]
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1918 +/- ##
===========================================
- Coverage 83.68% 71.99% -11.69%
===========================================
Files 145 141 -4
Lines 23830 17882 -5948
Branches 12 19 +7
===========================================
- Hits 19941 12875 -7066
- Misses 3889 5007 +1118
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
interesting approach in general - lgtm.
some observation:
-
running tests using
nimbleadd some time overhead. nimble command has to be compiled and run. this takes ~15s on my machine so running tests directly is faster.nim c -r -d:test_path="mix" ./tests/test_path ... is the same as ... nimble testpath mixmaybe
./tests/test_pathcan be./tests/test_allmaybe document running tests with
nim c -rcommand? -
nimblecan sometimes be pain in the ass... but looks avoidable with above command -
conditionally imported tests files that require build flags like
libp2p_autotls_support- we should always import these files, but if flag is not provided we should skip running suite
nim c -r -d:test_path="libp2p/autotls" ./tests/test_path
this should not fail, because we need to test what happens when flag is not provided.
^
we can address this in later PR.
leaving to richard and gabe to review and give feedback as well
libp2p/protocols/kademlia.nim
Outdated
| import ./kademlia/[routingtable, protobuf, types, find, get, put, provider, ping] | ||
|
|
||
| export routingtable, protobuf, types, find, get, put, provider, ping | ||
| export routingtable, protobuf, types, find, get, put, provider, ping, chronicles |
There was a problem hiding this comment.
could you check if this is needed after change in tests/libp2p/kademlia/test_encoding.nim?
also would be better to avoid merging code without this hack. or if we merge we have to create TODO and open issue to resolve this.
There was a problem hiding this comment.
Changes in tests/libp2p/kademlia/test_encoding.nim were caused by this one here. And exporting chronicles solved my issue, I'm not sure how else it can be resolved.
Is it a a problem though? I found more places in a codebase were chronicles are exported as well:
libp2p/protocols/perf/server.nim
libp2p/protocols/rendezvous/rendezvous.nim
libp2p/protocols/ping.nim
There was a problem hiding this comment.
it doesn't seem normal to me... we should not do that
There was a problem hiding this comment.
Any ideas how to solve it differently?
There was a problem hiding this comment.
Ok, it's reverted. I just used chronicles in tests as a workaround. Feel free to suggest something better, but this does the trick for now.
There was a problem hiding this comment.
I just used chronicles in tests as a workaround
could you please create issue that addresses this "hack"?
I found more places in a codebase were chronicles are exported as well:
could you also create issue that addresses this? we should not export chronicles here.
tests/imports.nim
Outdated
| let imports = newStmtList() | ||
| for file in walkDirRec(dir): | ||
| let (path, name, ext) = splitFile(file) | ||
| let isTestFile = name.startsWith("test_") and not name.startsWith("test_all") |
There was a problem hiding this comment.
name.startsWith("test_all") -> can be buggy if file name is "test_all_secure_channels".
perhaps name != "test_all" ?
There was a problem hiding this comment.
or maybe we dont need this anymore?
There was a problem hiding this comment.
It's needed to not import itself 😄
There was a problem hiding this comment.
test_path.nimis also running this logic, and this file has risk or including itselftest_all.nimdoes not have risk of including itself as search is limited inlibp2pandtoolsdirs
to make tool generally useful:
- files that are calling
buildImportsshould limit search to directories where anything withtest_*.nimfilename can be included. - if anything should be ignored then
ignorePathsshould be used - no special name handling should be done, as it can be error prone (in some cases here or other repositories)
all of above are just driving in direction where buildImports could be copy-pasted to different projects, or event merged to unittest2 as utility. and if unittest2 does not want it there, we should have it here but also keep it as general purpose.
@vladopajic I can document also using the |
yes, please. waiting on |
| echo "==================================" | ||
| echo "\n" | ||
|
|
||
| macro importTests*( |
There was a problem hiding this comment.
@arnetheduck hey, does it make sense to have importTests in unittest2 ?
|
LGTM |
Co-authored-by: richΛrd <info@richardramos.me>
Changes:
add
importTestsmacro to dynamically import test files instead of usingtest_allfiles with static importsnimble test- goes through all dirs and files insidetests/libp2p/andtests/tools/and imports all the tests (test_*.nim)nimble testpath <path>- can be used to selectively run tests that match<path>(see readme for examples)using dynamic imports uncovered an issue with
chroniclesinkademliatests - when test files didn't importchroniclesthen we got compilation errorundeclared identifier: 'activeChroniclesStream'but when they imported, then we gotchronicles not usederror - use chronicles in tests as a workaround