-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-44062: [Dev][Archery][Integration] Reduce needless test matrix #44099
Changes from all commits
80551c3
bad8c35
f39ed69
1055eb3
0567f1a
b092bc3
b3c9cb7
f4fd547
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -744,6 +744,9 @@ def _set_default(opt, default): | |
@click.option('--with-rust', type=bool, default=False, | ||
help='Include Rust in integration tests', | ||
envvar="ARCHERY_INTEGRATION_WITH_RUST") | ||
@click.option('--target-languages', default='', | ||
help=('Target languages in this integration tests'), | ||
envvar="ARCHERY_INTEGRATION_TARGET_LANGUAGES") | ||
Comment on lines
+747
to
+749
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uh, can the help string be more explanatory? It's really not possible to understand what this does from the current wording. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. But is the help string a suitable location to explain it more with click API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then it can be explained in the command's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I'll use it: GH-44158 |
||
@click.option('--write_generated_json', default="", | ||
help='Generate test JSON to indicated path') | ||
@click.option('--run-ipc', is_flag=True, default=False, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -67,12 +67,13 @@ class IntegrationRunner(object): | |||||
|
||||||
def __init__(self, json_files, | ||||||
flight_scenarios: List[Scenario], | ||||||
testers: List[Tester], tempdir=None, | ||||||
debug=False, stop_on_error=True, gold_dirs=None, | ||||||
testers: List[Tester], other_testers: List[Tester], | ||||||
tempdir=None, debug=False, stop_on_error=True, gold_dirs=None, | ||||||
serial=False, match=None, **unused_kwargs): | ||||||
self.json_files = json_files | ||||||
self.flight_scenarios = flight_scenarios | ||||||
self.testers = testers | ||||||
self.other_testers = other_testers | ||||||
self.temp_dir = tempdir or tempfile.mkdtemp() | ||||||
self.debug = debug | ||||||
self.stop_on_error = stop_on_error | ||||||
|
@@ -100,6 +101,20 @@ def run_ipc(self): | |||||
producer, consumer, self._produce_consume, | ||||||
self.json_files) | ||||||
|
||||||
for producer, consumer in itertools.product( | ||||||
filter(lambda t: t.PRODUCER, self.testers), | ||||||
filter(lambda t: t.CONSUMER, self.other_testers)): | ||||||
self._compare_ipc_implementations( | ||||||
producer, consumer, self._produce_consume, | ||||||
self.json_files) | ||||||
|
||||||
for producer, consumer in itertools.product( | ||||||
filter(lambda t: t.PRODUCER, self.other_testers), | ||||||
filter(lambda t: t.CONSUMER, self.testers)): | ||||||
self._compare_ipc_implementations( | ||||||
producer, consumer, self._produce_consume, | ||||||
self.json_files) | ||||||
|
||||||
if self.gold_dirs: | ||||||
for gold_dir, consumer in itertools.product( | ||||||
self.gold_dirs, | ||||||
|
@@ -124,7 +139,7 @@ def run_flight(self): | |||||
""" | ||||||
servers = filter(lambda t: t.FLIGHT_SERVER, self.testers) | ||||||
clients = filter(lambda t: (t.FLIGHT_CLIENT and t.CONSUMER), | ||||||
self.testers) | ||||||
self.testers + self.other_testers) | ||||||
for server, client in itertools.product(servers, clients): | ||||||
self._compare_flight_implementations(server, client) | ||||||
log('\n') | ||||||
|
@@ -138,6 +153,14 @@ def run_c_data(self): | |||||
filter(lambda t: t.C_DATA_SCHEMA_EXPORTER, self.testers), | ||||||
filter(lambda t: t.C_DATA_SCHEMA_IMPORTER, self.testers)): | ||||||
self._compare_c_data_implementations(producer, consumer) | ||||||
for producer, consumer in itertools.product( | ||||||
filter(lambda t: t.C_DATA_SCHEMA_EXPORTER, self.testers), | ||||||
filter(lambda t: t.C_DATA_SCHEMA_IMPORTER, self.other_testers)): | ||||||
self._compare_c_data_implementations(producer, consumer) | ||||||
for producer, consumer in itertools.product( | ||||||
filter(lambda t: t.C_DATA_SCHEMA_EXPORTER, self.other_testers), | ||||||
filter(lambda t: t.C_DATA_SCHEMA_IMPORTER, self.testers)): | ||||||
self._compare_c_data_implementations(producer, consumer) | ||||||
log('\n') | ||||||
|
||||||
def _gold_tests(self, gold_dir): | ||||||
|
@@ -560,31 +583,42 @@ def get_static_json_files(): | |||||
def run_all_tests(with_cpp=True, with_java=True, with_js=True, | ||||||
with_csharp=True, with_go=True, with_rust=False, | ||||||
with_nanoarrow=False, run_ipc=False, run_flight=False, | ||||||
run_c_data=False, tempdir=None, **kwargs): | ||||||
run_c_data=False, tempdir=None, target_languages="", | ||||||
**kwargs): | ||||||
tempdir = tempdir or tempfile.mkdtemp(prefix='arrow-integration-') | ||||||
print(["before", target_languages]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should remove those There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes... I've removed it: #44140 |
||||||
target_languages = list(filter(len, target_languages.split(","))) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would you pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I know that click supports There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you're talking about the empty string, sorry, I had misread.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I'll use it in #44156. |
||||||
print(["after", target_languages]) | ||||||
|
||||||
testers: List[Tester] = [] | ||||||
other_testers: List[Tester] = [] | ||||||
|
||||||
def append_tester(language, tester): | ||||||
if len(target_languages) == 0 or language in target_languages: | ||||||
testers.append(tester) | ||||||
else: | ||||||
other_testers.append(tester) | ||||||
|
||||||
if with_cpp: | ||||||
testers.append(CppTester(**kwargs)) | ||||||
append_tester("cpp", CppTester(**kwargs)) | ||||||
|
||||||
if with_java: | ||||||
testers.append(JavaTester(**kwargs)) | ||||||
append_tester("java", JavaTester(**kwargs)) | ||||||
|
||||||
if with_js: | ||||||
testers.append(JSTester(**kwargs)) | ||||||
append_tester("js", JSTester(**kwargs)) | ||||||
|
||||||
if with_csharp: | ||||||
testers.append(CSharpTester(**kwargs)) | ||||||
append_tester("csharp", CSharpTester(**kwargs)) | ||||||
|
||||||
if with_go: | ||||||
testers.append(GoTester(**kwargs)) | ||||||
append_tester("go", GoTester(**kwargs)) | ||||||
|
||||||
if with_nanoarrow: | ||||||
testers.append(NanoarrowTester(**kwargs)) | ||||||
append_tester("nanoarrow", NanoarrowTester(**kwargs)) | ||||||
|
||||||
if with_rust: | ||||||
testers.append(RustTester(**kwargs)) | ||||||
append_tester("rust", RustTester(**kwargs)) | ||||||
|
||||||
static_json_files = get_static_json_files() | ||||||
generated_json_files = datagen.get_generated_json_files(tempdir=tempdir) | ||||||
|
@@ -666,7 +700,8 @@ def run_all_tests(with_cpp=True, with_java=True, with_js=True, | |||||
), | ||||||
] | ||||||
|
||||||
runner = IntegrationRunner(json_files, flight_scenarios, testers, **kwargs) | ||||||
runner = IntegrationRunner(json_files, flight_scenarios, testers, | ||||||
other_testers, **kwargs) | ||||||
if run_ipc: | ||||||
runner.run_ipc() | ||||||
if run_flight: | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Languages" is not really right here, as nanoarrow for example is not a language. "Implementations" perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I'll rename it: GH-44155